On Thu, Jun 23, 2022 at 7:05 AM shiy.f...@fujitsu.com <shiy.f...@fujitsu.com> wrote: > > 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.
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