Refactor to use common function 'get_publications_str'.
Hi,
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.
This is a refactoring patch to make better use of the existing function.
Changes:
- The function has been moved and renamed to 'GetPublicationsStr'
because it is no longer static.
- Now function GetPublicationsStr is also being called from
tablesync.c, fetch_remote_table_info().
- I found fetch_remote_table_info() was building the same publication
list multiple times. In passing, fixed this to make the list only
once.
- Similarly, there was a duplicate list building code in
subscriptioncmds.c fetch_table_list(). Not a performance hit -- just
more code than needed. In passing, simplified this too.
~~~
Please take a look at the attached patch v1.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Refactor-to-use-common-function-GetPublicationsSt.patchapplication/octet-stream; name=v1-0001-Refactor-to-use-common-function-GetPublicationsSt.patchDownload
From d1c4a8c9a799714dce46193a1a8f4d2ec4814225 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 23 Oct 2024 15:33:30 +1100
Subject: [PATCH v1] Refactor to use common function 'GetPublicationsStr'.
There is already a utility function (called 'get_publications_str') for building
a comma-seaparated string of publication names, but it was not being used
everywhere it could have been.
This is a refactoring patch to make better use of this existing function.
Changes:
- The function has been moved, and renamed to 'GetPublicationsStr', because it
is no longer static.
- Now GetPublicationsStr() is also being called from tablesync.c,
fetch_remote_table_info().
- fetch_remote_table_info() was building the same publication list multiple
times. In passing, fixed this to make the list only once.
- Similarly, there was duplicated calls subscriptioncmds.c fetch_table_list().
This is not causing a performance hit -- just more code than was needed.
In passing, simplified this too.
---
src/backend/catalog/pg_subscription.c | 31 +++++++++++++++
src/backend/commands/subscriptioncmds.c | 60 +++++++----------------------
src/backend/replication/logical/tablesync.c | 35 +++--------------
src/include/catalog/pg_subscription.h | 5 ++-
4 files changed, 55 insertions(+), 76 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9efc915..f48d230 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -35,6 +35,37 @@
static List *textarray_to_stringlist(ArrayType *textarray);
/*
+ * Add publication names from the list to a string.
+ */
+void
+GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
+{
+ ListCell *lc;
+ bool first = true;
+
+ Assert(publications != NIL);
+
+ foreach(lc, publications)
+ {
+ char *pubname = strVal(lfirst(lc));
+
+ if (first)
+ first = false;
+ else
+ appendStringInfoString(dest, ", ");
+
+ if (quote_literal)
+ appendStringInfoString(dest, quote_literal_cstr(pubname));
+ else
+ {
+ appendStringInfoChar(dest, '"');
+ appendStringInfoString(dest, pubname);
+ appendStringInfoChar(dest, '"');
+ }
+ }
+}
+
+/*
* Fetch the subscription from the syscache.
*/
Subscription *
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 02ccc63..609e932 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,37 +440,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
}
/*
- * Add publication names from the list to a string.
- */
-static void
-get_publications_str(List *publications, StringInfo dest, bool quote_literal)
-{
- ListCell *lc;
- bool first = true;
-
- Assert(publications != NIL);
-
- foreach(lc, publications)
- {
- char *pubname = strVal(lfirst(lc));
-
- if (first)
- first = false;
- else
- appendStringInfoString(dest, ", ");
-
- if (quote_literal)
- appendStringInfoString(dest, quote_literal_cstr(pubname));
- else
- {
- appendStringInfoChar(dest, '"');
- appendStringInfoString(dest, pubname);
- appendStringInfoChar(dest, '"');
- }
- }
-}
-
-/*
* Check that the specified publications are present on the publisher.
*/
static void
@@ -486,7 +455,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoString(cmd, "SELECT t.pubname FROM\n"
" pg_catalog.pg_publication t WHERE\n"
" t.pubname IN (");
- get_publications_str(publications, cmd, true);
+ GetPublicationsStr(publications, cmd, true);
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
@@ -523,7 +492,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
/* Prepare the list of non-existent publication(s) for error message. */
StringInfo pubnames = makeStringInfo();
- get_publications_str(publicationsCopy, pubnames, false);
+ GetPublicationsStr(publicationsCopy, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg_plural("publication %s does not exist on the publisher",
@@ -2151,7 +2120,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
" JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
- get_publications_str(publications, &cmd, true);
+ GetPublicationsStr(publications, &cmd, true);
appendStringInfoString(&cmd, ")\n");
/*
@@ -2208,7 +2177,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
StringInfo pubnames = makeStringInfo();
/* Prepare the list of publication(s) for warning message. */
- get_publications_str(publist, pubnames, false);
+ GetPublicationsStr(publist, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin",
@@ -2243,17 +2212,17 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
List *tablelist = NIL;
int server_version = walrcv_server_version(wrconn);
bool check_columnlist = (server_version >= 150000);
+ StringInfo pub_names = makeStringInfo();
initStringInfo(&cmd);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(publications, pub_names, true);
+
/* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
- StringInfoData pub_names;
-
tableRow[2] = INT2VECTOROID;
- initStringInfo(&pub_names);
- get_publications_str(publications, &pub_names, true);
/*
* From version 16, we allowed passing multiple publications to the
@@ -2275,9 +2244,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
" FROM pg_publication\n"
" WHERE pubname IN ( %s )) AS gpt\n"
" ON gpt.relid = c.oid\n",
- pub_names.data);
-
- pfree(pub_names.data);
+ pub_names->data);
}
else
{
@@ -2288,12 +2255,13 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
if (check_columnlist)
appendStringInfoString(&cmd, ", t.attnames\n");
- appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
- " WHERE t.pubname IN (");
- get_publications_str(publications, &cmd, true);
- appendStringInfoChar(&cmd, ')');
+ appendStringInfo(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
+ " WHERE t.pubname IN ( %s )",
+ pub_names->data);
}
+ pfree(pub_names->data);
+
res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
pfree(cmd.data);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e03e761..9f52964 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -802,8 +802,8 @@ fetch_remote_table_info(char *nspname, char *relname,
Oid qualRow[] = {TEXTOID};
bool isnull;
int natt;
- ListCell *lc;
Bitmapset *included_cols = NULL;
+ StringInfo pub_names = makeStringInfo();
lrel->nspname = nspname;
lrel->relname = relname;
@@ -844,6 +844,8 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(MySubscription->publications, pub_names, true);
/*
* Get column lists for each relation.
@@ -856,15 +858,6 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
- StringInfoData pub_names;
-
- initStringInfo(&pub_names);
- foreach(lc, MySubscription->publications)
- {
- if (foreach_current_index(lc) > 0)
- appendStringInfoString(&pub_names, ", ");
- appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
- }
/*
* Fetch info about column lists for the relation (from all the
@@ -881,7 +874,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u AND c.oid = gpt.relid"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
pubres = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
lengthof(attrsRow), attrsRow);
@@ -936,8 +929,6 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(tslot);
walrcv_clear_result(pubres);
-
- pfree(pub_names.data);
}
/*
@@ -1039,20 +1030,6 @@ fetch_remote_table_info(char *nspname, char *relname,
*/
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
- StringInfoData pub_names;
-
- /* Build the pubname list. */
- initStringInfo(&pub_names);
- foreach_node(String, pubstr, MySubscription->publications)
- {
- char *pubname = strVal(pubstr);
-
- if (foreach_current_index(pubstr) > 0)
- appendStringInfoString(&pub_names, ", ");
-
- appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
- }
-
/* Check for row filters. */
resetStringInfo(&cmd);
appendStringInfo(&cmd,
@@ -1062,7 +1039,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data, 1, qualRow);
@@ -1103,7 +1080,7 @@ fetch_remote_table_info(char *nspname, char *relname,
walrcv_clear_result(res);
}
- pfree(cmd.data);
+ pfree(pub_names->data);
}
/*
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 0aa14ec..b25f3fe 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -20,7 +20,7 @@
#include "access/xlogdefs.h"
#include "catalog/genbki.h"
#include "catalog/pg_subscription_d.h"
-
+#include "lib/stringinfo.h"
#include "nodes/pg_list.h"
/*
@@ -180,4 +180,7 @@ extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
+extern void GetPublicationsStr(List *publications, StringInfo dest,
+ bool quote_literal);
+
#endif /* PG_SUBSCRIPTION_H */
--
1.8.3.1
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
Attachments:
v2-0001-Refactor-to-use-common-function-GetPublicationsSt.patchapplication/octet-stream; name=v2-0001-Refactor-to-use-common-function-GetPublicationsSt.patchDownload
From 2409514eb0766bae244fd2b0b6fc976ced26c19d Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 23 Oct 2024 18:19:14 +1100
Subject: [PATCH v2] Refactor to use common function 'GetPublicationsStr'.
There is already a utility function (called 'get_publications_str') for building
a comma-separated string of publication names, but it was not being used
everywhere it could have been.
This is a refactoring patch to make better use of this existing function.
Changes:
- The function has been moved, and renamed to 'GetPublicationsStr', because it
is no longer static.
- Now GetPublicationsStr() is also being called from tablesync.c,
fetch_remote_table_info().
- fetch_remote_table_info() was building the same publication list multiple
times. In passing, fixed this to make the list only once.
- Similarly, there was duplicated calls subscriptioncmds.c fetch_table_list().
This is not causing a performance hit -- just more code than was needed.
In passing, simplified this too.
---
src/backend/catalog/pg_subscription.c | 31 +++++++++++++++
src/backend/commands/subscriptioncmds.c | 60 +++++++----------------------
src/backend/replication/logical/tablesync.c | 34 +++-------------
src/include/catalog/pg_subscription.h | 5 ++-
4 files changed, 55 insertions(+), 75 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9efc915..f48d230 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -35,6 +35,37 @@
static List *textarray_to_stringlist(ArrayType *textarray);
/*
+ * Add publication names from the list to a string.
+ */
+void
+GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
+{
+ ListCell *lc;
+ bool first = true;
+
+ Assert(publications != NIL);
+
+ foreach(lc, publications)
+ {
+ char *pubname = strVal(lfirst(lc));
+
+ if (first)
+ first = false;
+ else
+ appendStringInfoString(dest, ", ");
+
+ if (quote_literal)
+ appendStringInfoString(dest, quote_literal_cstr(pubname));
+ else
+ {
+ appendStringInfoChar(dest, '"');
+ appendStringInfoString(dest, pubname);
+ appendStringInfoChar(dest, '"');
+ }
+ }
+}
+
+/*
* Fetch the subscription from the syscache.
*/
Subscription *
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 02ccc63..f7f6701 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,37 +440,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
}
/*
- * Add publication names from the list to a string.
- */
-static void
-get_publications_str(List *publications, StringInfo dest, bool quote_literal)
-{
- ListCell *lc;
- bool first = true;
-
- Assert(publications != NIL);
-
- foreach(lc, publications)
- {
- char *pubname = strVal(lfirst(lc));
-
- if (first)
- first = false;
- else
- appendStringInfoString(dest, ", ");
-
- if (quote_literal)
- appendStringInfoString(dest, quote_literal_cstr(pubname));
- else
- {
- appendStringInfoChar(dest, '"');
- appendStringInfoString(dest, pubname);
- appendStringInfoChar(dest, '"');
- }
- }
-}
-
-/*
* Check that the specified publications are present on the publisher.
*/
static void
@@ -486,7 +455,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoString(cmd, "SELECT t.pubname FROM\n"
" pg_catalog.pg_publication t WHERE\n"
" t.pubname IN (");
- get_publications_str(publications, cmd, true);
+ GetPublicationsStr(publications, cmd, true);
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
@@ -523,7 +492,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
/* Prepare the list of non-existent publication(s) for error message. */
StringInfo pubnames = makeStringInfo();
- get_publications_str(publicationsCopy, pubnames, false);
+ GetPublicationsStr(publicationsCopy, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg_plural("publication %s does not exist on the publisher",
@@ -2151,7 +2120,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
" JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
- get_publications_str(publications, &cmd, true);
+ GetPublicationsStr(publications, &cmd, true);
appendStringInfoString(&cmd, ")\n");
/*
@@ -2208,7 +2177,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
StringInfo pubnames = makeStringInfo();
/* Prepare the list of publication(s) for warning message. */
- get_publications_str(publist, pubnames, false);
+ GetPublicationsStr(publist, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin",
@@ -2243,17 +2212,17 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
List *tablelist = NIL;
int server_version = walrcv_server_version(wrconn);
bool check_columnlist = (server_version >= 150000);
+ StringInfo pub_names = makeStringInfo();
initStringInfo(&cmd);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(publications, pub_names, true);
+
/* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
- StringInfoData pub_names;
-
tableRow[2] = INT2VECTOROID;
- initStringInfo(&pub_names);
- get_publications_str(publications, &pub_names, true);
/*
* From version 16, we allowed passing multiple publications to the
@@ -2275,9 +2244,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
" FROM pg_publication\n"
" WHERE pubname IN ( %s )) AS gpt\n"
" ON gpt.relid = c.oid\n",
- pub_names.data);
-
- pfree(pub_names.data);
+ pub_names->data);
}
else
{
@@ -2288,12 +2255,13 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
if (check_columnlist)
appendStringInfoString(&cmd, ", t.attnames\n");
- appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
- " WHERE t.pubname IN (");
- get_publications_str(publications, &cmd, true);
- appendStringInfoChar(&cmd, ')');
+ appendStringInfo(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
+ " WHERE t.pubname IN ( %s )",
+ pub_names->data);
}
+ destroyStringInfo(pub_names);
+
res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
pfree(cmd.data);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e03e761..e0cbf11 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -802,8 +802,8 @@ fetch_remote_table_info(char *nspname, char *relname,
Oid qualRow[] = {TEXTOID};
bool isnull;
int natt;
- ListCell *lc;
Bitmapset *included_cols = NULL;
+ StringInfo pub_names = makeStringInfo();
lrel->nspname = nspname;
lrel->relname = relname;
@@ -844,6 +844,8 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(MySubscription->publications, pub_names, true);
/*
* Get column lists for each relation.
@@ -856,15 +858,6 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
- StringInfoData pub_names;
-
- initStringInfo(&pub_names);
- foreach(lc, MySubscription->publications)
- {
- if (foreach_current_index(lc) > 0)
- appendStringInfoString(&pub_names, ", ");
- appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
- }
/*
* Fetch info about column lists for the relation (from all the
@@ -881,7 +874,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u AND c.oid = gpt.relid"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
pubres = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
lengthof(attrsRow), attrsRow);
@@ -936,8 +929,6 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(tslot);
walrcv_clear_result(pubres);
-
- pfree(pub_names.data);
}
/*
@@ -1039,20 +1030,6 @@ fetch_remote_table_info(char *nspname, char *relname,
*/
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
- StringInfoData pub_names;
-
- /* Build the pubname list. */
- initStringInfo(&pub_names);
- foreach_node(String, pubstr, MySubscription->publications)
- {
- char *pubname = strVal(pubstr);
-
- if (foreach_current_index(pubstr) > 0)
- appendStringInfoString(&pub_names, ", ");
-
- appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
- }
-
/* Check for row filters. */
resetStringInfo(&cmd);
appendStringInfo(&cmd,
@@ -1062,7 +1039,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data, 1, qualRow);
@@ -1103,6 +1080,7 @@ fetch_remote_table_info(char *nspname, char *relname,
walrcv_clear_result(res);
}
+ destroyStringInfo(pub_names);
pfree(cmd.data);
}
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 0aa14ec..b25f3fe 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -20,7 +20,7 @@
#include "access/xlogdefs.h"
#include "catalog/genbki.h"
#include "catalog/pg_subscription_d.h"
-
+#include "lib/stringinfo.h"
#include "nodes/pg_list.h"
/*
@@ -180,4 +180,7 @@ extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
+extern void GetPublicationsStr(List *publications, StringInfo dest,
+ bool quote_literal);
+
#endif /* PG_SUBSCRIPTION_H */
--
1.8.3.1
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
Attachments:
v3-0001-Refactor-to-use-common-function-GetPublicationsSt.patchapplication/octet-stream; name=v3-0001-Refactor-to-use-common-function-GetPublicationsSt.patchDownload
From 9b6b8a2f9e3c583c68f558d12f74b9945da4254e Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 24 Oct 2024 09:07:38 +1100
Subject: [PATCH v3] Refactor to use common function 'GetPublicationsStr'.
There is already a utility function (called 'get_publications_str') for building
a comma-separated string of publication names, but it was not being used
everywhere it could have been.
This is a refactoring patch to make better use of this existing function.
Changes:
- The function has been moved, and renamed to 'GetPublicationsStr', because it
is no longer static.
- Now GetPublicationsStr() is also being called from tablesync.c,
fetch_remote_table_info().
- fetch_remote_table_info() was building the same publication list multiple
times. In passing, fixed this to make the list only once.
- Similarly, there were duplicated calls subscriptioncmds.c fetch_table_list().
This was not causing a performance hit -- just more code than was needed.
In passing, simplified this too.
---
src/backend/catalog/pg_subscription.c | 31 +++++++++++++++
src/backend/commands/subscriptioncmds.c | 60 +++++++----------------------
src/backend/replication/logical/tablesync.c | 34 +++-------------
src/include/catalog/pg_subscription.h | 5 ++-
4 files changed, 55 insertions(+), 75 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9efc915..89bf5ec 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -35,6 +35,37 @@
static List *textarray_to_stringlist(ArrayType *textarray);
/*
+ * Add a comma-separated list of publication names to the 'dest' string.
+ */
+void
+GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
+{
+ ListCell *lc;
+ bool first = true;
+
+ Assert(publications != NIL);
+
+ foreach(lc, publications)
+ {
+ char *pubname = strVal(lfirst(lc));
+
+ if (first)
+ first = false;
+ else
+ appendStringInfoString(dest, ", ");
+
+ if (quote_literal)
+ appendStringInfoString(dest, quote_literal_cstr(pubname));
+ else
+ {
+ appendStringInfoChar(dest, '"');
+ appendStringInfoString(dest, pubname);
+ appendStringInfoChar(dest, '"');
+ }
+ }
+}
+
+/*
* Fetch the subscription from the syscache.
*/
Subscription *
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 02ccc63..f7f6701 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,37 +440,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
}
/*
- * Add publication names from the list to a string.
- */
-static void
-get_publications_str(List *publications, StringInfo dest, bool quote_literal)
-{
- ListCell *lc;
- bool first = true;
-
- Assert(publications != NIL);
-
- foreach(lc, publications)
- {
- char *pubname = strVal(lfirst(lc));
-
- if (first)
- first = false;
- else
- appendStringInfoString(dest, ", ");
-
- if (quote_literal)
- appendStringInfoString(dest, quote_literal_cstr(pubname));
- else
- {
- appendStringInfoChar(dest, '"');
- appendStringInfoString(dest, pubname);
- appendStringInfoChar(dest, '"');
- }
- }
-}
-
-/*
* Check that the specified publications are present on the publisher.
*/
static void
@@ -486,7 +455,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoString(cmd, "SELECT t.pubname FROM\n"
" pg_catalog.pg_publication t WHERE\n"
" t.pubname IN (");
- get_publications_str(publications, cmd, true);
+ GetPublicationsStr(publications, cmd, true);
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
@@ -523,7 +492,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
/* Prepare the list of non-existent publication(s) for error message. */
StringInfo pubnames = makeStringInfo();
- get_publications_str(publicationsCopy, pubnames, false);
+ GetPublicationsStr(publicationsCopy, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg_plural("publication %s does not exist on the publisher",
@@ -2151,7 +2120,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
" JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
- get_publications_str(publications, &cmd, true);
+ GetPublicationsStr(publications, &cmd, true);
appendStringInfoString(&cmd, ")\n");
/*
@@ -2208,7 +2177,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
StringInfo pubnames = makeStringInfo();
/* Prepare the list of publication(s) for warning message. */
- get_publications_str(publist, pubnames, false);
+ GetPublicationsStr(publist, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin",
@@ -2243,17 +2212,17 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
List *tablelist = NIL;
int server_version = walrcv_server_version(wrconn);
bool check_columnlist = (server_version >= 150000);
+ StringInfo pub_names = makeStringInfo();
initStringInfo(&cmd);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(publications, pub_names, true);
+
/* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
- StringInfoData pub_names;
-
tableRow[2] = INT2VECTOROID;
- initStringInfo(&pub_names);
- get_publications_str(publications, &pub_names, true);
/*
* From version 16, we allowed passing multiple publications to the
@@ -2275,9 +2244,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
" FROM pg_publication\n"
" WHERE pubname IN ( %s )) AS gpt\n"
" ON gpt.relid = c.oid\n",
- pub_names.data);
-
- pfree(pub_names.data);
+ pub_names->data);
}
else
{
@@ -2288,12 +2255,13 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
if (check_columnlist)
appendStringInfoString(&cmd, ", t.attnames\n");
- appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
- " WHERE t.pubname IN (");
- get_publications_str(publications, &cmd, true);
- appendStringInfoChar(&cmd, ')');
+ appendStringInfo(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
+ " WHERE t.pubname IN ( %s )",
+ pub_names->data);
}
+ destroyStringInfo(pub_names);
+
res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
pfree(cmd.data);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e03e761..e0cbf11 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -802,8 +802,8 @@ fetch_remote_table_info(char *nspname, char *relname,
Oid qualRow[] = {TEXTOID};
bool isnull;
int natt;
- ListCell *lc;
Bitmapset *included_cols = NULL;
+ StringInfo pub_names = makeStringInfo();
lrel->nspname = nspname;
lrel->relname = relname;
@@ -844,6 +844,8 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(MySubscription->publications, pub_names, true);
/*
* Get column lists for each relation.
@@ -856,15 +858,6 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
- StringInfoData pub_names;
-
- initStringInfo(&pub_names);
- foreach(lc, MySubscription->publications)
- {
- if (foreach_current_index(lc) > 0)
- appendStringInfoString(&pub_names, ", ");
- appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
- }
/*
* Fetch info about column lists for the relation (from all the
@@ -881,7 +874,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u AND c.oid = gpt.relid"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
pubres = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
lengthof(attrsRow), attrsRow);
@@ -936,8 +929,6 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(tslot);
walrcv_clear_result(pubres);
-
- pfree(pub_names.data);
}
/*
@@ -1039,20 +1030,6 @@ fetch_remote_table_info(char *nspname, char *relname,
*/
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
- StringInfoData pub_names;
-
- /* Build the pubname list. */
- initStringInfo(&pub_names);
- foreach_node(String, pubstr, MySubscription->publications)
- {
- char *pubname = strVal(pubstr);
-
- if (foreach_current_index(pubstr) > 0)
- appendStringInfoString(&pub_names, ", ");
-
- appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
- }
-
/* Check for row filters. */
resetStringInfo(&cmd);
appendStringInfo(&cmd,
@@ -1062,7 +1039,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data, 1, qualRow);
@@ -1103,6 +1080,7 @@ fetch_remote_table_info(char *nspname, char *relname,
walrcv_clear_result(res);
}
+ destroyStringInfo(pub_names);
pfree(cmd.data);
}
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 0aa14ec..b25f3fe 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -20,7 +20,7 @@
#include "access/xlogdefs.h"
#include "catalog/genbki.h"
#include "catalog/pg_subscription_d.h"
-
+#include "lib/stringinfo.h"
#include "nodes/pg_list.h"
/*
@@ -180,4 +180,7 @@ extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
+extern void GetPublicationsStr(List *publications, StringInfo dest,
+ bool quote_literal);
+
#endif /* PG_SUBSCRIPTION_H */
--
1.8.3.1
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
Attachments:
v4-0001-Refactor-to-use-common-function-GetPublicationsSt.patchapplication/octet-stream; name=v4-0001-Refactor-to-use-common-function-GetPublicationsSt.patchDownload
From 6ddf93eedfff98e591a6b8fc9b609337b05cf9e2 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Fri, 25 Oct 2024 09:05:36 +1100
Subject: [PATCH v4] Refactor to use common function 'GetPublicationsStr'.
There is already a utility function (called 'get_publications_str') for building
a comma-separated string of publication names, but it was not being used
everywhere it could have been.
This is a refactoring patch to make better use of this existing function.
Changes:
- The function has been moved, and renamed to 'GetPublicationsStr', because it
is no longer static.
- Now GetPublicationsStr() is also being called from tablesync.c,
fetch_remote_table_info().
- fetch_remote_table_info() was building the same publication string multiple
times. In passing, fixed to build the string only once.
- Similarly, there were duplicated calls subscriptioncmds.c fetch_table_list().
This was not causing a performance hit -- just more code than was needed.
In passing, simplified this too.
---
src/backend/catalog/pg_subscription.c | 31 +++++++++++++++
src/backend/commands/subscriptioncmds.c | 60 +++++++----------------------
src/backend/replication/logical/tablesync.c | 35 +++++------------
src/include/catalog/pg_subscription.h | 5 ++-
4 files changed, 58 insertions(+), 73 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9efc915..89bf5ec 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -35,6 +35,37 @@
static List *textarray_to_stringlist(ArrayType *textarray);
/*
+ * Add a comma-separated list of publication names to the 'dest' string.
+ */
+void
+GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
+{
+ ListCell *lc;
+ bool first = true;
+
+ Assert(publications != NIL);
+
+ foreach(lc, publications)
+ {
+ char *pubname = strVal(lfirst(lc));
+
+ if (first)
+ first = false;
+ else
+ appendStringInfoString(dest, ", ");
+
+ if (quote_literal)
+ appendStringInfoString(dest, quote_literal_cstr(pubname));
+ else
+ {
+ appendStringInfoChar(dest, '"');
+ appendStringInfoString(dest, pubname);
+ appendStringInfoChar(dest, '"');
+ }
+ }
+}
+
+/*
* Fetch the subscription from the syscache.
*/
Subscription *
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 02ccc63..f7f6701 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,37 +440,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
}
/*
- * Add publication names from the list to a string.
- */
-static void
-get_publications_str(List *publications, StringInfo dest, bool quote_literal)
-{
- ListCell *lc;
- bool first = true;
-
- Assert(publications != NIL);
-
- foreach(lc, publications)
- {
- char *pubname = strVal(lfirst(lc));
-
- if (first)
- first = false;
- else
- appendStringInfoString(dest, ", ");
-
- if (quote_literal)
- appendStringInfoString(dest, quote_literal_cstr(pubname));
- else
- {
- appendStringInfoChar(dest, '"');
- appendStringInfoString(dest, pubname);
- appendStringInfoChar(dest, '"');
- }
- }
-}
-
-/*
* Check that the specified publications are present on the publisher.
*/
static void
@@ -486,7 +455,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
appendStringInfoString(cmd, "SELECT t.pubname FROM\n"
" pg_catalog.pg_publication t WHERE\n"
" t.pubname IN (");
- get_publications_str(publications, cmd, true);
+ GetPublicationsStr(publications, cmd, true);
appendStringInfoChar(cmd, ')');
res = walrcv_exec(wrconn, cmd->data, 1, tableRow);
@@ -523,7 +492,7 @@ check_publications(WalReceiverConn *wrconn, List *publications)
/* Prepare the list of non-existent publication(s) for error message. */
StringInfo pubnames = makeStringInfo();
- get_publications_str(publicationsCopy, pubnames, false);
+ GetPublicationsStr(publicationsCopy, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg_plural("publication %s does not exist on the publisher",
@@ -2151,7 +2120,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
" JOIN pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n"
" pg_class C JOIN pg_namespace N ON (N.oid = C.relnamespace)\n"
"WHERE C.oid = GPT.relid AND P.pubname IN (");
- get_publications_str(publications, &cmd, true);
+ GetPublicationsStr(publications, &cmd, true);
appendStringInfoString(&cmd, ")\n");
/*
@@ -2208,7 +2177,7 @@ check_publications_origin(WalReceiverConn *wrconn, List *publications,
StringInfo pubnames = makeStringInfo();
/* Prepare the list of publication(s) for warning message. */
- get_publications_str(publist, pubnames, false);
+ GetPublicationsStr(publist, pubnames, false);
ereport(WARNING,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("subscription \"%s\" requested copy_data with origin = NONE but might copy data that had a different origin",
@@ -2243,17 +2212,17 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
List *tablelist = NIL;
int server_version = walrcv_server_version(wrconn);
bool check_columnlist = (server_version >= 150000);
+ StringInfo pub_names = makeStringInfo();
initStringInfo(&cmd);
+ /* Build the pub_names comma-separated string. */
+ GetPublicationsStr(publications, pub_names, true);
+
/* Get the list of tables from the publisher. */
if (server_version >= 160000)
{
- StringInfoData pub_names;
-
tableRow[2] = INT2VECTOROID;
- initStringInfo(&pub_names);
- get_publications_str(publications, &pub_names, true);
/*
* From version 16, we allowed passing multiple publications to the
@@ -2275,9 +2244,7 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
" FROM pg_publication\n"
" WHERE pubname IN ( %s )) AS gpt\n"
" ON gpt.relid = c.oid\n",
- pub_names.data);
-
- pfree(pub_names.data);
+ pub_names->data);
}
else
{
@@ -2288,12 +2255,13 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
if (check_columnlist)
appendStringInfoString(&cmd, ", t.attnames\n");
- appendStringInfoString(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
- " WHERE t.pubname IN (");
- get_publications_str(publications, &cmd, true);
- appendStringInfoChar(&cmd, ')');
+ appendStringInfo(&cmd, "FROM pg_catalog.pg_publication_tables t\n"
+ " WHERE t.pubname IN ( %s )",
+ pub_names->data);
}
+ destroyStringInfo(pub_names);
+
res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
pfree(cmd.data);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e03e761..e8bf736 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -802,7 +802,7 @@ fetch_remote_table_info(char *nspname, char *relname,
Oid qualRow[] = {TEXTOID};
bool isnull;
int natt;
- ListCell *lc;
+ StringInfo pub_names = NULL;
Bitmapset *included_cols = NULL;
lrel->nspname = nspname;
@@ -856,15 +856,10 @@ fetch_remote_table_info(char *nspname, char *relname,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
- StringInfoData pub_names;
- initStringInfo(&pub_names);
- foreach(lc, MySubscription->publications)
- {
- if (foreach_current_index(lc) > 0)
- appendStringInfoString(&pub_names, ", ");
- appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
- }
+ /* Build the pub_names comma-separated string. */
+ pub_names = makeStringInfo();
+ GetPublicationsStr(MySubscription->publications, pub_names, true);
/*
* Fetch info about column lists for the relation (from all the
@@ -881,7 +876,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u AND c.oid = gpt.relid"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
pubres = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
lengthof(attrsRow), attrsRow);
@@ -936,8 +931,6 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(tslot);
walrcv_clear_result(pubres);
-
- pfree(pub_names.data);
}
/*
@@ -1039,19 +1032,8 @@ fetch_remote_table_info(char *nspname, char *relname,
*/
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
- StringInfoData pub_names;
-
- /* Build the pubname list. */
- initStringInfo(&pub_names);
- foreach_node(String, pubstr, MySubscription->publications)
- {
- char *pubname = strVal(pubstr);
-
- if (foreach_current_index(pubstr) > 0)
- appendStringInfoString(&pub_names, ", ");
-
- appendStringInfoString(&pub_names, quote_literal_cstr(pubname));
- }
+ /* Reuse the already built pub_names comma-separated string. */
+ Assert(pub_names);
/* Check for row filters. */
resetStringInfo(&cmd);
@@ -1062,7 +1044,7 @@ fetch_remote_table_info(char *nspname, char *relname,
" WHERE gpt.relid = %u"
" AND p.pubname IN ( %s )",
lrel->remoteid,
- pub_names.data);
+ pub_names->data);
res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data, 1, qualRow);
@@ -1101,6 +1083,7 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecDropSingleTupleTableSlot(slot);
walrcv_clear_result(res);
+ destroyStringInfo(pub_names);
}
pfree(cmd.data);
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 0aa14ec..b25f3fe 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -20,7 +20,7 @@
#include "access/xlogdefs.h"
#include "catalog/genbki.h"
#include "catalog/pg_subscription_d.h"
-
+#include "lib/stringinfo.h"
#include "nodes/pg_list.h"
/*
@@ -180,4 +180,7 @@ extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
+extern void GetPublicationsStr(List *publications, StringInfo dest,
+ bool quote_literal);
+
#endif /* PG_SUBSCRIPTION_H */
--
1.8.3.1
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