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

On Wed, Apr 7, 2021 at 10:37 PM vignesh C <vignesh21(at)gmail(dot)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