Refactor to use common function 'get_publications_str'.

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.

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

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.

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

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 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, ')');

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

~~

Please see the attached patch v3.

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

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

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

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 Thu, Oct 24, 2024 at 5:44 PM Peter Smith <smithpb2250@gmail.com> wrote:

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.

I've attached the patch v4.

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

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

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