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

On Wed, Jul 12, 2023 at 8:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > 10.
> > + /* 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 am not sure if the function IsIndexOnlyExpression() even needed
> > anymore. Isn't it sufficient just to check up-front is the leftmost
> > INDEX field is a column and that covers this condition also? Actually,
> > this same question is also open in the Sawada-san thread [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. 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."

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. We
can probably have an assert for this as well.

[1] - https://www.postgresql.org/message-id/CAA4eK1Jv8%2B8rax-bejd3Ej%2BT2fG3tuqP8v89byKCBPVQj9C9pg%40mail.gmail.com

--
With Regards,
Amit Kapila.