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