Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

Hi Vignesh C,

>
> Few comments
> 1) Maybe this change is not required:
> fallback if no other solution is possible. If a replica identity other
> than <quote>full</quote> is set on the publisher side, a replica
> identity
> - comprising the same or fewer columns must also be set on the subscriber
> - side. See <xref linkend="sql-altertable-replica-identity"/> for
> details on
> + comprising the same or fewer columns must also be set on the
> subscriber side.
> + See <xref linkend="sql-altertable-replica-identity"/> for details on
>

Yes, fixed.

>
> 2) Variable declaration and the assignment can be split so that the
> readability is better:
> +
> + bool isUsableIndex =
> + IsIndexUsableForReplicaIdentityFull(indexInfo);
> +
> + index_close(indexRelation, AccessShareLock);
> +

Hmm, can you please elaborate more on this? The declaration
and assignment are already on different lines.

ps: pgindent changed this line a bit. Does that look better?

3) Since there is only one statement within the if condition, the
> braces can be removed
> + if (is_btree && !is_partial && !is_only_on_expression)
> + {
> + return true;
> + }
>
>
Fixed on a newer version of the patch. Now it is only:

*return is_btree && !is_partial && !is_only_on_expression;*

> 4) There is minor indentation issue in this, we could run pgindent to fix
> it:
> +static Oid FindLogicalRepLocalIndex(Relation localrel,
> +
> LogicalRepRelation *remoterel);
> +
>
>
Yes, pgindent fixed it, thanks.

Attached v37

Thanks,
Onder KALACI