Re: Identify missing publications from publisher while create/alter subscription.

On Thu, May 6, 2021 at 9:08 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
>
> On Tue, 04 May 2021 at 21:20, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > On Tue, May 4, 2021 at 2:37 PM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>
> >> On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21(at)gmail(dot)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, "\"");

Modified.

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

Modified.

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

I felt this list entries will be deleted in the success case, in error
case I felt no need to delete it as we will be exiting. Thoughts?

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

Modified.

Thanks for the comments, attached v9 patch has the fixes for the same.

Regards,
Vignesh