Re: Refactor to use common function 'get_publications_str'.

On Wed, Oct 23, 2024 at 3:26 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Thu, Oct 24, 2024 at 8:26 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> Thanks for your feedback!
>
> >
> > While the changes look good to me, the comment of GetPublicationsStr()
> > seems not match what the function actually does:
> >
> > +/*
> > + * Add publication names from the list to a string.
> > + */
> > +void
> > +GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
> >
> > It's true that the function adds publication names to the given
> > StringInfo, but it seems to me that the function expects the string is
> > empty. For example, if we call this function twice with the same
> > publication list, say 'pub1' and 'pub2', it would return a string
> > 'pub1,pub2pub1,pub2'.
>
> No, although this function is not designed to be called twice in a
> row, there are code examples where this function is being called
> passing (non-empty) SQL cmd string.
>
> e.g.
> cmd = makeStringInfo();
> appendStringInfoString(cmd, "SELECT t.pubname FROM\n"
> " pg_catalog.pg_publication t WHERE\n"
> " t.pubname IN (");
> GetPublicationsStr(publications, cmd, true);
> appendStringInfoChar(cmd, ')');

Thanks, that makes sense.

>
> > I think we can improve the description of this
> > function to something like "Build a comma-separated string from the
> > given list of publication names.".
> >
>
> This is a refactoring patch, so I hadn't intended to touch the
> function, but I agree the function comment could be better.
>
> Now, I've changed the function comment to:
> /*
> * Add a comma-separated list of publication names to the 'dest' string.
> */

Thank you for updating the patch. The patch looks good to me.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com