public inbox for [email protected]  

help / color / mirror / Atom feed

From: Masahiko Sawada <[email protected]>
To: Peter Smith <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Refactor to use common function 'get_publications_str'.
Date: Wed, 23 Oct 2024 15:40:19 -0700
Message-ID: <CAD21AoCf6c9WAA2fdvCF14so-BffK1B1JAWnxTX-Cm8Q0gJH5w@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PshwcOdq+MbgEM2ardEvCVXHxL-6SDzZo4aA=v+qgr7iA@mail.gmail.com>
References: <CAHut+PtJMk4bKXqtpvqVy9ckknCgK9P6=FeG8zHF=6+Em_Snpw@mail.gmail.com>
	<[email protected]>
	<CAHut+PuVEHu62iWpBwnMEwxLYKEA-1TB888Mm1qB6rjbbXn+fw@mail.gmail.com>
	<CAD21AoARPYOP+RTn7UZ8Ybja-HW5aU3N=aNeo7uD2gGG-ZS21g@mail.gmail.com>
	<CAHut+PshwcOdq+MbgEM2ardEvCVXHxL-6SDzZo4aA=v+qgr7iA@mail.gmail.com>

On Wed, Oct 23, 2024 at 3:26 PM Peter Smith <[email protected]> wrote:
>
> On Thu, Oct 24, 2024 at 8:26 AM Masahiko Sawada <[email protected]> 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

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Refactor to use common function 'get_publications_str'.
  In-Reply-To: <CAD21AoCf6c9WAA2fdvCF14so-BffK1B1JAWnxTX-Cm8Q0gJH5w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox