Re: Handle infinite recursion in logical replication setup

On Thu, Jun 23, 2022 at 7:05 AM shiy(dot)fnst(at)fujitsu(dot)com
<shiy(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Mon, Jun 20, 2022 7:55 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Thanks for the comment, the v22 patch attached has the changes for the
> > same.
>
> Thanks for updating the patch, here are some comments on 0003 patch.
>
> 1. 032_origin.pl
> +###############################################################################
> +# Join 3rd node (node_C) to the existing 2 nodes(node_A & node_B) bidirectional
> +# replication setup when the existing nodes (node_A & node_B) and the new node
> +# (node_C) does not have any data.
> +###############################################################################
>
> "does not have any data" should be "do not have any data" I think.

I felt the existing is ok, I did not find it gramatically wrong. I did
not make any changes for this.

> 2.
> The comment for 032_origin.pl:
>
> # Test the CREATE SUBSCRIPTION 'origin' parameter.
>
> After applying this patch, this file tests no only 'origin' parameter, but also
> "copy_data" parameter, so should we modify this comment?

Modified

> Besides, should we change the file name in this patch? It looks more like test
> cases for bidirectional logical replication.

I felt let's keep it as origin, as most of the tests are based on
'origin' parameter, also later features will be able to filter a
particular origin, those tests can easily fit in the same file.

> 3. subscriptioncmds.c
> /* Set default values for the boolean supported options. */
> ...
> if (IsSet(supported_opts, SUBOPT_CREATE_SLOT))
> opts->create_slot = true;
> if (IsSet(supported_opts, SUBOPT_COPY_DATA))
> - opts->copy_data = true;
> + opts->copy_data = COPY_DATA_ON;
> if (IsSet(supported_opts, SUBOPT_REFRESH))
> opts->refresh = true;
> if (IsSet(supported_opts, SUBOPT_BINARY))
>
> "copy_data" option is not Boolean now, which is inconsistent with the comment.
> So maybe we can change the comment here? ("the boolean supported options" ->
> "the supported options")

Modified

Thanks for the comments, the v23 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm1-ZrG%3DhaAoiB2yFKYc%2Bckcd1NLaU8QB3SWs32wPsph4w%40mail.gmail.com

Regards,
Vignesh