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

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