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

On Thu, Jul 13, 2023 at 8:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
>

I have followed the later style in the attached.

> 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?
>

For now, I have kept the assert but moved it to the end of the function.

Apart from the above, I have made a number of minor changes (a)
changed the datatype for the strategy to StrategyNumber at various
places in the patch; (b) made a number of changes in comments based on
Peter's comments and otherwise; (c) ran pgindent and changed the
commit message; (d) few other cosmetic changes.

Let me know what you think of the attached.

--
With Regards,
Amit Kapila.