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

On Tue, Apr 13, 2021 at 8:01 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 6:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > 2) How about
> > > +          Specifies whether the subscriber must verify the
> > > publications that are
> > > +       being subscribed to are present in the publisher. By default,
> > > the subscriber
> > > instead of
> > > +          Specifies whether the subscriber must verify if the specified
> > > +          publications are present in the publisher. By default, the subscriber
> > >
> >
> > Slightly reworded and modified.
>
> +         <para>
> +          When true, the command will try to verify if the specified
> +          publications that are subscribed is present in the publisher.
> +          The default is <literal>false</literal>.
> +         </para>
>
> "publications that are subscribed" is not right as the subscriber is
> not yet subscribed, it is "trying to subscribing", and it's not that
> the command "will try to verify", it actually verifies. So you can
> modify as follows:
>
> +         <para>
> +          When true, the command verifies if all the specified
> publications that are being subscribed to are present in the publisher
> and throws an error if any of the publication doesn't exist. The
> default is <literal>false</literal>.
> +         </para>
>
> > > 3) I think we can make below common code into a single function with
> > > flags to differentiate processing for both, something like:
> > > StringInfoData *get_publist_str(List *publicaitons, bool use_quotes,
> > > bool is_fetch_table_list);
> > > check_publications:
> > > +        /* Convert the publications which does not exist into a string. */
> > > +        initStringInfo(&nonExistentPublications);
> > > +        foreach(lc, publicationsCopy)
> > > +        {
> > > and get_appended_publications_query:
> > >      foreach(lc, publications)
> > >
> > > With the new function that only prepares comma separated list of
> > > publications, you can get rid of get_appended_publications_query and
> > > just append the returned list to the query.
> > > fetch_table_list: get_publist_str(publications, true, true);
> > > check_publications: for select query preparation
> > > get_publist_str(publications, true, false); and for error string
> > > preparation get_publist_str(publications, false, false);
> > >
> > > And also let the new function get_publist_str allocate the string and
> > > just mention as a note in the function comment that the callers should
> > > pfree the returned string.
> > >
> >
> > I felt the existing code looks better, if we have a common function,
> > we will have to lot of if conditions as both the functions is not same
> > to same, they operate on different data types and do the preparation
> > appropriately. Like fetch_table_list get nspname & relname and
> > converts it to RangeVar and adds to the list other function prepares a
> > text and deletes the entries present from the list. So I did not fix
> > this. Thoughts?
>
> I was actually thinking we could move the following duplicate code
> into a function:
>         foreach(lc, publicationsCopy)
>         {
>             char       *pubname = strVal(lfirst(lc));
>
>             if (first)
>                 first = false;
>             else
>                 appendStringInfoString(&pubnames, ", ");
>             appendStringInfoString(&pubnames, "\"");
>             appendStringInfoString(&pubnames, pubname);
>             appendStringInfoString(&pubnames, "\"");
>         }
> and
>     foreach(lc, publications)
>     {
>         char       *pubname = strVal(lfirst(lc));
>
>         if (first)
>             first = false;
>         else
>             appendStringInfoString(cmd, ", ");
>
>         appendStringInfoString(cmd, quote_literal_cstr(pubname));
>     }
> that function can be:
> static void
> get_publications_str(List *publications, StringInfo dest, bool quote_literal)
> {
>     ListCell   *lc;
>     bool        first = true;
>
>     Assert(list_length(publications) > 0);
>
>     foreach(lc, publications)
>     {
>         char       *pubname = strVal(lfirst(lc));
>
>         if (first)
>             first = false;
>         else
>             appendStringInfoString(dest, ", ");
>
>         if (quote_literal)
>             appendStringInfoString(pubnames, quote_literal_cstr(pubname));
>         else
>         {
>             appendStringInfoString(&dest, "\"");
>             appendStringInfoString(&dest, pubname);
>             appendStringInfoString(&dest, "\"");
>         }
>     }
> }
>
> This way, we can get rid of get_appended_publications_query and use
> the above function to return the appended list of publications. We
> need to just pass quote_literal as true while preparing the publist
> string for publication query and append it to the query outside the
> function. While preparing publist str for error, pass quote_literal as
> false. Thoughts?
>

Modified.

> > > 7) You can just do
> > > publications = list_copy(publications);
> > > instead of using another variable publicationsCopy
> > >     publicationsCopy = list_copy(publications);
> >
> > publications is an input list to this function, I did not want this
> > function to change this list. I felt existing is fine. Thoughts?
>
> Okay.
>
> Typo - it's not "subcription" +# Create subcription for a publication
> which does not exist.
>

Modified

> I think we can remove extra { } by moving the comment above if clause
> much like you did in AlterSubscription_refresh. And it's not "exists",
> it is "exist" change in both AlterSubscription_refresh and
> CreateSubscription.
> +            if (validate_publication)
> +            {
> +                /* Verify specified publications exists in the publisher. */
> +                check_publications(wrconn, publications);
> +            }
> +

Modified.

>
> Move /*no streaming */ to above NULL, NULL line:
> +                                           NULL, NULL,
>                                             NULL, NULL); /* no streaming */
>

Modified.

> Can we have a new function for below duplicate code? Something like:
> void connect_and_check_pubs(Subscription *sub, List *publications);?
> +                if (validate_publication)
> +                {
> +                    /* Load the library providing us libpq calls. */
> +                    load_file("libpqwalreceiver", false);
> +
> +                    /* Try to connect to the publisher. */
> +                    wrconn = walrcv_connect(sub->conninfo, true,
> sub->name, &err);
> +                    if (!wrconn)
> +                        ereport(ERROR,
> +                                (errmsg("could not connect to the
> publisher: %s", err)));
> +
> +                    /* Verify specified publications exists in the
> publisher. */
> +                    check_publications(wrconn, stmt->publication);
> +
> +                    /* We are done with the remote side, close connection. */
> +                    walrcv_disconnect(wrconn);
> +                }
Modified.

Thanks for the comments, Attached patch has the fixes for the same.
Thoughts?

Regards,
Vignesh