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

On Wed, Apr 7, 2021 at 10:37 PM vignesh C <vignesh21@gmail.com> wrote:
> > I think, we can also have validate_publication option allowed for
> > ALTER SUBSCRIPTION SET PUBLICATION and REFRESH PUBLICATION commands
> > with the same behaviour i.e. error out when specified publications
> > don't exist in the publisher. Thoughts?
>
> Sorry for the delayed reply, I was working on a few other projects so
> I was not able to reply quickly.
> Since we are getting the opinion that if we make the check
> publications by default it might affect the existing users, I'm fine
> with having an option validate_option to check if the publication
> exists in the publisher, that way there is no change for the existing
> user. I have made a patch in similar lines, attached patch has the
> changes for the same.
> Thoughts?

Here are some comments on v3 patch:

1) Please mention what's the default value of the option
+       <varlistentry>
+        <term><literal>validate_publication</literal>
(<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether the subscriber must verify if the specified
+          publications are present in the publisher. By default, the subscriber
+          will not check if the publications are present in the publisher.
+         </para>
+        </listitem>
+       </varlistentry>

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

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.

4) We could do following,
        ereport(ERROR,
                (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
                 errmsg_plural("publication %s does not exist in the publisher",
                           "publications %s do not exist in the publisher",
                        list_length(publicationsCopy),
                        nonExistentPublications.data)));
instead of
+        ereport(ERROR,
+                (errmsg("publication(s) %s does not exist in the publisher",
+                        nonExistentPublications.data)));

        if (list_member(cte->ctecolnames,
makeString(cte->search_clause->search_seq_column)))

5) I think it's better with
+ * Check the specified publication(s) is(are) present in the publisher.
instead of
+ * Verify that the specified publication(s) exists in the publisher.

6) Instead of such a longer variable name "nonExistentPublications"
how about just "pubnames" and add a comment there saying "going to
error out with the list of non-existent publications" with that the
variable and that part of code's context is clear.

7) You can just do
publications = list_copy(publications);
instead of using another variable publicationsCopy
    publicationsCopy = list_copy(publications);

8) If you have done StringInfoData *cmd = makeStringInfo();, then no
need of initStringInfo(cmd);

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