Re: Refactor to use common function 'get_publications_str'.

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, ')');

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

~~

Please see the attached patch v3.

======
Kind Regards,
Peter Smith.
Fujitsu Australia