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
