Re: Identify missing publications from publisher while create/alter subscription. - Mailing list pgsql-hackers

On Tue, 04 May 2021 at 21:20, vignesh C <vignesh21@gmail.com> wrote:
> On Tue, May 4, 2021 at 2:37 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21@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.
>>
>
> Modified.
>
>> 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.
>>
>
> Modified.
>
>> 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);
>>     }
>>
>
> Modified.
>
>> 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. */
>>
>
> Modified.
>
>> 5) A typo -  it is "do not exist"
>> # Multiple publications does not exist.
>>
>
> Modified.
>
>> 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/,
>
> Modified.
>
> Thanks for the comments, Attached patch has the fixes for the same.

Thanks for updating the patch.  Some comments on v8 patch.

1) How about use appendStringInfoChar() to replace the first and last one,
since it more faster.
+                       appendStringInfoString(dest, "\"");
+                       appendStringInfoString(dest, pubname);
+                       appendStringInfoString(dest, "\"");

2) How about use if (!validate_publication) to keep the code style consistent?
+       if (validate_publication == false)
+               return;

3) Should we free the memory when finish the check_publications()?
+       publicationsCopy = list_copy(publications);

4) It is better wrap the word "streaming" with quote.  Also, should we add
'no "validate_publication"' comment for validate_publication parameters?
                                                                                   NULL, NULL,  /* no "binary" */
-                                                                                  NULL, NULL); /* no streaming */
+                                                                                  NULL, NULL,  /* no streaming */
+                                                                                  NULL, NULL);

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.