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

On Thu, Apr 8, 2021 at 12:13 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> 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:
>

Thanks for the comments

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

Modified.

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

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

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

Modified.

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

Modified.

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

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?

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

Modified.

Attached v4 patch has the fixes for the comments.

Regards,
Vignesh