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 21:25:40 |
Message-ID: | CAD21AoARPYOP+RTn7UZ8Ybja-HW5aU3N=aNeo7uD2gGG-ZS21g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Oct 23, 2024 at 12:25 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Oct 23, 2024 at 5:23 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >
> > On Wed, Oct 23, 2024 at 03:44:03PM +1100, Peter Smith wrote:
> > > During a code review, it was noticed that there are several places
> > > within logical replication where a comma-separated list of publication
> > > names is built explicitly. There is already a utility function (called
> > > 'get_publications_str') for doing this, but it was not being used in
> > > every place it could have been.
> >
> > Agreed that this is a good idea, saving from some duplication in the
> > tablesync code where the same thing is done, with the quoting on top
> > of that.
> >
>
> Thanks for your review and feedback!
>
> > - pfree(cmd.data);
> > + pfree(pub_names->data);
> >
> > The pfree for cmd.data should still be here, no? And you would need a
> > pfree(pub_names) as well, meaning that this could just use
> > destroyStringInfo().
>
> My bad, fixed in patch v2.
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'. 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.".
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2024-10-23 21:29:50 | Re: Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH |
Previous Message | Melanie Plageman | 2024-10-23 21:21:14 | Re: Trigger more frequent autovacuums of heavy insert tables |