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

On Thu, May 6, 2021 at 9:08 AM Japin Li <japinli@hotmail.com> wrote:
>
>
> 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, "\"");

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