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

Here are my review comments for the patch v4.

======
General

1.
FYI, this patch also needs some minor PG docs updates [1] because
section "31.1 Publication" currently refers to only btree - e.g.
"Candidate indexes must be btree, non-partial, and have..."

(this may also clash with Sawada-san's other thread as previously mentioned)

======
Commit message

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.

~

Typo:
/"amgetuple"/"amgettuple"/

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

3.
+ * 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

3a.
Typo:
/"amgetuple"/"amgettuple"/

~

3b.
I think my previous comment about this comment was misunderstood. I
was questioning why that last sentence ("To make it...") about
"index_getbitmap()" is even needed in this patch. I thought it may be
overreach to describe details how to make some future enhancement that
might never happen.


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

4. IsIndexUsableForReplicaIdentityFull

 IsIndexUsableForReplicaIdentityFull(IndexInfo *indexInfo)
 {
- bool is_btree = (indexInfo->ii_Am == BTREE_AM_OID);
- bool is_partial = (indexInfo->ii_Predicate != NIL);
- bool is_only_on_expression = IsIndexOnlyOnExpression(indexInfo);
+ bool is_suitable_type;
+ bool is_partial;
+ bool is_only_on_expression;

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

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.

~~~

4b.

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

The above statement has an extra set of nested parentheses for some reason.

======
src/backend/utils/cache/lsyscache.c

5.
/*
 * get_opclass_method
 *
 * Returns the OID of the index access method operator class is for.
 */
Oid
get_opclass_method(Oid opclass)

IMO the comment should be worded more like the previous one in this source file.

SUGGESTION
Returns the OID of the index access method the opclass belongs to.

------
[1] https://www.postgresql.org/docs/devel/logical-replication-publication.html

Kind Regards,
Peter Smith.
Fujitsu Australia