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

Hi Hayato, all

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

> 4a.
> > The code can be written more optimally, because if it is deemed
> > already that 'is_suitable_type' is false, then there is no point to
> > continue to do the other checks and function calls.

Sure, there are maybe few cycles of CPU gains, but this code is executed
infrequently, and I don't see much value optimizing it. I think keeping it
slightly
more readable is nicer.

Other than that, I think the code/test looks good. For the
comments/documentation,
I think Amit and Peter have already given quite a bit of useful feedback,
so nothing
much to add from my end.

Thanks,
Onder

[1]:
https://www.postgresql.org/message-id/CACawEhUWH1qAZ8QNeCve737Qe1_ye%3DvTW9P22ePiFssT7%2BHaaQ%40mail.gmail.com

Hayato Kuroda (Fujitsu) <kuroda(dot)hayato(at)fujitsu(dot)com>, 12 Tem 2023 Çar, 10:07
tarihinde şunu yazdı:

> Dear Amit,
>
> Thanks for checking my patch! The patch can be available at [1].
>
> > > > ======
> > > > .../subscription/t/032_subscribe_use_index.pl
> > > >
> > > > 11.
> > > > AFAIK there is no test to verify that the leftmost index field must
> be
> > > > a column (e.g. cannot be an expression). Yes, there are some tests
> for
> > > > *only* expressions like (expr, expr, expr), but IMO there should be
> > > > another test for an index like (expr, expr, column) which should also
> > > > fail because the column is not the leftmost field.
> > >
> > > I'm not sure they should be fixed in the patch. You have raised these
> points
> > > at the Sawada-san's thread[1] and not sure he has done.
> > > Furthermore, these points are not related with our initial motivation.
> > > Fork, or at least it should be done by another patch.
> > >
> >
> > I think it is reasonable to discuss the existing code-related
> > improvements in a separate thread rather than trying to change this
> > patch.
>
> OK, so I will not touch in this thread.
>
> > I have some other comments about your patch:
> >
> > 1.
> > + * 1) Other indexes do not have a fixed set of strategy numbers.
> > + * In build_replindex_scan_key(), the operator which corresponds to
> "equality
> > + * comparison" is searched by using the strategy number, but it is
> difficult
> > + * for such indexes. For example, GiST index operator classes for
> > + * two-dimensional geometric objects have a comparison "same", but its
> > strategy
> > + * number is different from the gist_int4_ops, which is a GiST index
> operator
> > + * class that implements the B-tree equivalent.
> > + *
> > ...
> > + */
> > +int
> > +get_equal_strategy_number_for_am(Oid am)
> > ...
> >
> > I think this comment is slightly difficult to understand. Can we make
> > it a bit generic by saying something like: "The index access methods
> > other than BTREE and HASH don't have a fixed strategy for equality
> > operation. Note that in other index access methods, the support
> > routines of each operator class interpret the strategy numbers
> > according to the operator class's definition. So, we return
> > InvalidStrategy number for index access methods other than BTREE and
> > HASH."
>
> Sounds better. Fixed with some adjustments.
>
> > 2.
> > + * 2) Moreover, BRIN and GIN indexes do not implement "amgettuple".
> > + * RelationFindReplTupleByIndex() assumes that tuples will be fetched
> one by
> > + * one via index_getnext_slot(), but this currently requires the
> "amgetuple"
> > + * function. To make it use index_getbitmap() must be used, which
> fetches all
> > + * the tuples at once.
> > + */
> > +int
> > +get_equal_strategy_number_for_am(Oid am)
> > {
> > ..
> >
> > I don't think this is a good place for such a comment. We can probably
> > move this atop IsIndexUsableForReplicaIdentityFull(). I think you need
> > to mention two reasons in IsIndexUsableForReplicaIdentityFull() why we
> > support only BTREE and HASH index access methods (a) Refer to comments
> > atop get_equal_strategy_number_for_am(); (b) mention the reason
> > related to tuples_equal() as discussed in email [1]. Then you can say
> > that we also need to ensure that the index access methods that we
> > support must have an implementation "amgettuple" as later while
> > searching tuple via RelationFindReplTupleByIndex, we need the same.
>
> Fixed, and based on that I modified the commit message accordingly.
> How do you feel?
>
> > We can probably have an assert for this as well.
>
> Added.
>
> [1]:
> https://www.postgresql.org/message-id/TYAPR01MB5866B4F938ADD7088379633CF536A%40TYAPR01MB5866.jpnprd01.prod.outlook.com
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
>
>
>