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

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?

> > 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.

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

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

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

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com