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

On Tue, Apr 13, 2021 at 8:01 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, Apr 13, 2021 at 6:22 PM vignesh C <vignesh21(at)gmail(dot)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