Thread: Re: Refactor to use common function 'get_publications_str'.
On Wed, Oct 23, 2024 at 12:25 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Wed, Oct 23, 2024 at 5:23 PM Michael Paquier <michael@paquier.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
On Wed, Oct 23, 2024 at 3:26 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Oct 24, 2024 at 8:26 AM Masahiko Sawada <sawada.mshk@gmail.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
On Thu, Oct 24, 2024 at 3:17 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Oct 23, 2024 at 03:40:19PM -0700, Masahiko Sawada wrote: > > > 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. > > + /* Build the pub_names comma-separated string. */ > + GetPublicationsStr(MySubscription->publications, pub_names, true); > > In fetch_remote_table_info(), it is true that we may finish by > building the same list two times, but your patch also changes the > logic so as the string is built for nothing when dealing with a server > version of 14 or older. That's a waste in these cases. > -- Yes, well spotted -- I was aware of that. Originally I had coded a >= PG15 check for that pub_names assignment. e.g. if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000) GetPublicationsStr(MySubscription->publications, pub_names, true); But, somehow it seemed messy to do that just to cater for something I thought was not particularly likely. OTOH, I am happy to put that check back in if you think it is warranted. ====== Kind RegArds, Peter Smith. Fujitsu Australia
On Thu, Oct 24, 2024 at 5:41 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Oct 24, 2024 at 05:27:25PM +1100, Peter Smith wrote: > > Yes, well spotted -- I was aware of that. Originally I had coded a >= > > PG15 check for that pub_names assignment. e.g. > > > > if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000) > > GetPublicationsStr(MySubscription->publications, pub_names, true); > > I was wondering about putting the call of GetPublicationsStr() in the > first block with the makeStringInfo(), have an Assert checking that > pub_names->data is never NULL in the second block, and destroy the > StringInfo only if it has been allocated. > > > But, somehow it seemed messy to do that just to cater for something I > > thought was not particularly likely. OTOH, I am happy to put that > > check back in if you think it is warranted. > > I think I would add it. The publication list is not mandatory, but > your patch makes it look so. > -- No problem. I will post an updated patch tomorrow. ====== Kind Regards, Peter Smith. Fujitsu Australia
On Fri, Oct 25, 2024 at 10:00 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Oct 25, 2024 at 09:28:47AM +1100, Peter Smith wrote: > > I've attached the patch v4. > > Looks OK to me. Thanks. I'll see to get that done through the day. > -- Thanks for pushing! ====== Kind Regards, Peter Smith. Fujitsu Australia