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

Here are some review comments for the v5 patch

======
Commit message.

1.
89e46d allowed using indexes other than PRIMARY KEY or REPLICA IDENTITY on the
subscriber, but only the BTree index could be used. This commit extends the
limitation, now the Hash index can be also used. These 2 types of
indexes are the
only ones supported because only these
- use fix strategy numbers
- implement the "equality" strategy
- supported by tuples_equal()
- implement the function amgettuple()

~

1a.
/fix/fixed/

~

1b.
/supported by tuples_equal()/are supported by tuples_equal()/

~~~

2.
Index access methods other than them 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. 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.

~

/Index access methods other than them/Other index access methods/

~~~

3.
tuples_equal() cannot accept a datatype that does not have an operator class for
Btree or Hash. One motivation for other types of indexes to be used is to define
an index to attributes such as datatypes like point and box. But
lookup_type_cache(),
which is called from tuples_equal(), cannot return the valid value if
input datatypes
do not have such a opclass.

~

That paragraph looks the same as the code comment in
IsIndexUsableForReplicaIdentityFull(). I wrote some review comments
about that (see #5d below) so the same maybe applies here too.

======
src/backend/executor/execReplication.c

4.
+/*
+ * Return the strategy number which corresponds to the equality operator for
+ * given index access method.
+ *
+ * XXX: Currently, only Btree and Hash indexes are supported. This is because
+ * index access methods other than them 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 the InvalidStrategy number
+ * for index access methods other than BTREE and HASH.
+ */
+int
+get_equal_strategy_number_for_am(Oid am)

The function comment seems a bit long. Maybe it can be simplified a bit:

SUGGESTION
Return the strategy number which corresponds to the equality operator
for the given index access method, otherwise, return InvalidStrategy.

XXX: Currently, only Btree and Hash indexes are supported. The other
index access methods don't have a fixed strategy for equality
operation - instead, the support routines of each operator class
interpret the strategy numbers according to the operator class's
definition.

======
src/backend/replication/logical/relation.c

5. FindUsableIndexForReplicaIdentityFull

/*
* Returns true if the index is usable for replica identity full. For details,
* see FindUsableIndexForReplicaIdentityFull.
+ *
+ * XXX: Currently, only Btree and Hash indexes can be returned as usable one.
+ * This is because mainly two reasons:
+ *
+ * 1) Other index access methods other than Btree and Hash don't have a fixed
+ * strategy for equality operation. Refer comments atop
+ * get_equal_strategy_number_for_am.
+ * 2) tuples_equal cannot accept a datatype that does not have an operator
+ * class for Btree or Hash. One motivation for other types of indexes to be
+ * used is to define an index to attributes such as datatypes like point and
+ * box. But lookup_type_cache, which is called from tuples_equal, cannot return
+ * the valid value if input datatypes do not have such a opclass.
+ *
+ * Furthermore, 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 "amgettuple"
+ * function.
*/

5a.
/as usable one./as useable./

~

5b.
/Other index access methods other than Btree and Hash/Other index
access methods/

~

5c.
Maybe a blank line before 2) will help readability.

~

5d.
"One motivation for other types of indexes to be used is to define an
index to attributes such as datatypes like point and box."

Isn't it enough to omit that sentence and just say:

SUGGESTION
2) tuples_equal() cannot accept a datatype (e.g. point or box) that
does not have an operator class for Btree or Hash. This is because
lookup_type_cache(), which is called from tuples_equal(), cannot
return the valid value if input datatypes do not have such an opclass.

~~~

6. FindUsableIndexForReplicaIdentityFull

+ /* Check whether the index is supported or not */
+ if (get_equal_strategy_number_for_am(indexInfo->ii_Am) == InvalidStrategy)
+ return false;

6a.

Really, this entire function is for checking "is supported or not" so
IMO this is not the correct comment just for this statement. BTW, I
noted Onder suggested keeping this as a variable assignment (called
'has_equal_strategy') [1]. I also think having a variable is better
because then this extra comment would be unnecessary.

~

6b.
IMO the code is readable with the early exit, but it is fine also if
you want to revert it to how Onder suggested [1]. I think it is not
worth worrying too much here because it seems the Sawada-san patch [2]
might have intentions to refactor all this same function anyhow.

------
[1] Onder suggestion -
https://www.postgresql.org/message-id/CACawEhXvVqxoaqj5aanaT02DHYUJwpkssS4RTZRSuqEOpT0zQg%40mail.gmail.com
[2] Sawada-san other thread -
https://www.postgresql.org/message-id/CAD21AoAKx%2BFY4OPPj%2BMEF0gM-TAV0%3Dfd3EfPoEsa%2BcMQLiWjyA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia