Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL

On Wed, Jul 12, 2023 at 8:14 PM Önder Kalacı <onderkalaci@gmail.com> wrote:
>
>>
>> > - return is_btree && !is_partial && !is_only_on_expression;
>> > + /* Check whether the index is supported or not */
>> > + is_suitable_type = ((get_equal_strategy_number_for_am(indexInfo->ii_Am)
>> > + != InvalidStrategy));
>> > +
>> > + is_partial = (indexInfo->ii_Predicate != NIL);
>> > + is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
>> > +
>> > + return is_suitable_type && !is_partial && !is_only_on_expression;
>> >
>
>
> I don't want to repeat this too much, as it is a minor note. Just
> sharing my perspective here.
>
> As discussed in the other email [1], I feel like keeping
> IsIndexUsableForReplicaIdentityFull()  function readable is useful
> for documentation purposes as well.
>
> So, I'm more inclined to see something like your old code, maybe with
> a different variable name.
>
>> bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
>
>
> to
>>
>> bool has_equal_strategy = get_equal_strategy_number_for_am...
>> ....
>>  return  has_equal_strategy   && !is_partial && !is_only_on_expression;
>

+1 for the readability. I think we can as you suggest or I feel it
would be better if we return false as soon as we found any condition
is false. The current patch has a mixed style such that for
InvalidStrategy, it returns immediately but for others, it does a
combined check. The other point we should consider in this regard is
the below assert check:

+#ifdef USE_ASSERT_CHECKING
+ {
+ /* Check that the given index access method has amgettuple routine */
+ IndexAmRoutine *amroutine = GetIndexAmRoutineByAmId(indexInfo->ii_Am,
+ false);
+ Assert(amroutine->amgettuple != NULL);
+ }
+#endif

Apart from having an assert, we have the following two options (a)
check this condition as well and return false if am doesn't support
amgettuple (b) report elog(ERROR, ..) in this case.

I am of the opinion that we should either have an assert for this or
do (b) because if do (a) currently there is no case where it can
return false. What do you think?

--
With Regards,
Amit Kapila.