MIME-Version: 1.0
References: <CAHut+PtJMk4bKXqtpvqVy9ckknCgK9P6=FeG8zHF=6+Em_Snpw@mail.gmail.com>
 <ZxiWNJsp5xAd5UrD@paquier.xyz> <CAHut+PuVEHu62iWpBwnMEwxLYKEA-1TB888Mm1qB6rjbbXn+fw@mail.gmail.com>
 <CAD21AoARPYOP+RTn7UZ8Ybja-HW5aU3N=aNeo7uD2gGG-ZS21g@mail.gmail.com> <CAHut+PshwcOdq+MbgEM2ardEvCVXHxL-6SDzZo4aA=v+qgr7iA@mail.gmail.com>
In-Reply-To: <CAHut+PshwcOdq+MbgEM2ardEvCVXHxL-6SDzZo4aA=v+qgr7iA@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 23 Oct 2024 15:40:19 -0700
Message-ID: <CAD21AoCf6c9WAA2fdvCF14so-BffK1B1JAWnxTX-Cm8Q0gJH5w@mail.gmail.com>
Subject: Re: Refactor to use common function 'get_publications_str'.
To: Peter Smith <smithpb2250@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoCf6c9WAA2fdvCF14so-BffK1B1JAWnxTX-Cm8Q0gJH5w%40mail.gmail.com>
Precedence: bulk

On Wed, Oct 23, 2024 at 3:26=E2=80=AFPM Peter Smith <smithpb2250@gmail.com>=
 wrote:
>
> On Thu, Oct 24, 2024 at 8:26=E2=80=AFAM Masahiko Sawada <sawada.mshk@gmai=
l.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_lit=
eral)
> >
> > 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 =3D 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,

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