Re: Refactor to use common function 'get_publications_str'.

On Thu, Oct 24, 2024 at 3:17 PM Michael Paquier <michael(at)paquier(dot)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