Subject: Re: Refactor to use common function 'get_publications_str'.

From: Masahiko Sawada <[email-masked]>
Date: Wed, 23 Oct 2024 15:40:19 -0700

On Wed, Oct 23, 2024 at 3:26 PM Peter Smith <[email-masked]> wrote:
>
> On Thu, Oct 24, 2024 at 8:26 AM Masahiko Sawada <[email-masked]> 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: Michael Paquier <[email-masked]>
Date: Thu, 24 Oct 2024 13:17:39 +0900

> 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.
--
Michael

---

From: Peter Smith <[email-masked]>
Date: Thu, 24 Oct 2024 17:27:25 +1100

> 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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 24 Oct 2024 15:41:15 +0900

>   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.

> 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.
--
Michael

---

From: Peter Smith <[email-masked]>
Date: Thu, 24 Oct 2024 17:44:44 +1100

> your patch makes it look so.

No problem. I will post an updated patch tomorrow.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Fri, 25 Oct 2024 09:28:47 +1100

> No problem. I will post an updated patch tomorrow.

I've attached the patch v4.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Michael Paquier <[email-masked]>
Date: Fri, 25 Oct 2024 07:59:50 +0900

> I've attached the patch v4.

Looks OK to me.  Thanks.  I'll see to get that done through the day.
--
Michael

---

From: Peter Smith <[email-masked]>
Date: Fri, 25 Oct 2024 16:03:31 +1100

> Looks OK to me.  Thanks.  I'll see to get that done through the day.

Thanks for pushing!

======
Kind Regards,
Peter Smith.
Fujitsu Australia