Re: Refactor to use common function 'get_publications_str'.

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