Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL
Date
Msg-id CAA4eK1JLi_N0NPoNC_4yrrqff3Hxp9e62RD1-dco+Wqmctg98w@mail.gmail.com
Whole thread Raw
In response to Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: [Patch] Use *other* indexes on the subscriber when REPLICA IDENTITY is FULL
List pgsql-hackers
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.

Attachment

pgsql-hackers by date:

Previous
From: Xiaoran Wang
Date:
Subject: About `GetNonHistoricCatalogSnapshot`: does it allow developers to use catalog snapshot to scan non-catalog tables?
Next
From: Richard Guo
Date:
Subject: Re: Check lateral references within PHVs for memoize cache keys