On Mon, May 3, 2021 at 7:59 PM vignesh C <vignes...@gmail.com> wrote: > Thanks for the comments, these comments are handle in the v7 patch > posted in my earlier mail.
Thanks. Some comments on v7 patch: 1) How about "Add publication names from the list to a string." instead of * Append the list of publication to dest string. 2) How about "Connect to the publisher and see if the given publication(s) is(are) present." instead of * Connect to the publisher and check if the publication(s) exist. 3) Below comments are unnecessary as the functions/code following them will tell what the code does. /* Verify specified publication(s) exist in the publisher. */ /* We are done with the remote side, close connection. */ /* Verify specified publication(s) exist in the publisher. */ PG_TRY(); { check_publications(wrconn, publications, true); } PG_FINALLY(); { /* We are done with the remote side, close connection. */ walrcv_disconnect(wrconn); } 4) And also the comment below that's there before check_publications is unnecessary, as the function name and description would say it all. /* Verify specified publication(s) exist in the publisher. */ 5) A typo - it is "do not exist" # Multiple publications does not exist. 6) Should we use "m" specified in all the test cases something like we do for $stderr =~ m/threads are not supported on this platform/ or m/replication slot "test_slot" was not created in this database/? $stderr =~ /ERROR: publication "non_existent_pub" does not exist in the publisher/, With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com