On Thu, Jul 13, 2023 at 8:51 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jul 12, 2023 at 8:14 PM Önder Kalacı <onderkal...@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.
v6-0001-Allow-the-use-of-a-hash-index-on-the-subscriber-d.patch
Description: Binary data
