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

Dear Peter,

Thanks for giving comment.

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

Fixed that, but I could not find any other points. Do you have in mind others?
I checked related commits like 89e46d and adedf5, but only the part was changed.

> 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"/

Fixed.

> 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"/

Per suggestion from Amit [1], the paragraph was removed.

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

Removed.

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

Right, is_suitable_type is now removed.

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

This part was removed per above comment.

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

Fixed.

[1]: https://www.postgresql.org/message-id/CAA4eK1%2B%2BR3WSfsGH0yFR9WEbkDfF__OccADR7qXDnHGTww%2BkvQ%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED