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