From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Refactor to use common function 'get_publications_str'. |
Date: | 2024-10-23 22:40:19 |
Message-ID: | CAD21AoCf6c9WAA2fdvCF14so-BffK1B1JAWnxTX-Cm8Q0gJH5w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Jacob Champion | 2024-10-23 22:40:57 | Re: [PoC] Federated Authn/z with OAUTHBEARER |
Previous Message | David G. Johnston | 2024-10-23 22:40:17 | Re: Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH |