RE: Handle infinite recursion in logical replication setup

On Mon, Jun 20, 2022 7:55 PM vignesh C <vignesh21@gmail.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.

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?

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

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

Regards,
Shi yu