public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Eisentraut <[email protected]>
To: Euler Taveira <[email protected]>
To: Amit Kapila <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: FabrÃzio de Royes Mello <[email protected]>
Cc: vignesh C <[email protected]>
Subject: Re: speed up a logical replica setup
Date: Mon, 18 Mar 2024 14:52:28 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<TYCPR01MB12077A8421685E5515DE408EEF5512@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<CALDaNm2ev8_r4N5Sfa2GjzuzZNz17L0nsX_ZqfcarmS5oa5Szw@mail.gmail.com>
<TYCPR01MB12077CD333376B53F9CAE7AC0F5562@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<TYCPR01MB12077121263EFF7680486B635F5212@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<[email protected]>
<TYCPR01MB12077DE77454D68E959E85E35F5272@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<CANhcyEW6-dH28gLbFc5XpDTJ6JPizU+t5g-aKUWJBf5W_Zriqw@mail.gmail.com>
<[email protected]>
<CAA4eK1JtnzqYNQ_S+mj_XsPfgk7szxegKiVQq4-8fY1wUcUnpg@mail.gmail.com>
<[email protected]>
On 16.03.24 16:42, Euler Taveira wrote:
> I'm attaching a new version (v30) that adds:
I have some review comments and attached a patch with some smaller
fixups (mainly message wording and avoid fixed-size string buffers).
* doc/src/sgml/ref/pg_createsubscriber.sgml
I would remove the "How It Works" section. This is not relevant to
users, and it is very detailed and will require updating whenever the
implementation changes. It could be a source code comment instead.
* src/bin/pg_basebackup/pg_createsubscriber.c
I think the connection string handling is not robust against funny
characters, like spaces, in database names etc.
Most SQL commands need to be amended for proper identifier or string
literal quoting and/or escaping.
In check_subscriber(): All these permissions checks seem problematic
to me. We shouldn't reimplement our own copy of the server's
permission checks. The server can check the permissions. And if the
permission checking in the server ever changes, then we have
inconsistencies to take care of. Also, the error messages "permission
denied" are inappropriate, because we are not doing the actual thing.
Maybe we want to do a dry-run for the benefit of the user, but then we
should do the actual thing, like try to create a replication slot, or
whatever. But I would rather just remove all this, it seems too
problematic.
In main(): The first check if the standby is running is problematic.
I think it would be better to require that the standby is initially
shut down. Consider, the standby might be running under systemd.
This tool will try to stop it, systemd will try to restart it. Let's
avoid these kinds of battles. It's also safer if we don't try to
touch running servers.
The -p option (--subscriber-port) doesn't seem to do anything. In my
testing, it always uses the compiled-in default port.
Printing all the server log lines to the terminal doesn't seem very
user-friendly. Not sure what to do about that, short of keeping a
pg_upgrade-style directory of log files. But it's ugly.
From ec8e6ed6c3325a6f9fde2d1632346e212ade9c9f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Mon, 18 Mar 2024 14:48:55 +0100
Subject: [PATCH] Various improvements
---
src/bin/pg_basebackup/Makefile | 2 +-
src/bin/pg_basebackup/nls.mk | 1 +
src/bin/pg_basebackup/pg_createsubscriber.c | 49 +++++++++------------
3 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile
index e9a920dbcda..26c53e473f5 100644
--- a/src/bin/pg_basebackup/Makefile
+++ b/src/bin/pg_basebackup/Makefile
@@ -49,7 +49,7 @@ all: pg_basebackup pg_createsubscriber pg_receivewal pg_recvlogical
pg_basebackup: $(BBOBJS) $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $(BBOBJS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_createsubscriber: $(WIN32RES) pg_createsubscriber.o | submake-libpq submake-libpgport submake-libpgfeutils
+pg_createsubscriber: pg_createsubscriber.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
pg_receivewal: pg_receivewal.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
diff --git a/src/bin/pg_basebackup/nls.mk b/src/bin/pg_basebackup/nls.mk
index fc475003e8e..7870cea71ce 100644
--- a/src/bin/pg_basebackup/nls.mk
+++ b/src/bin/pg_basebackup/nls.mk
@@ -8,6 +8,7 @@ GETTEXT_FILES = $(FRONTEND_COMMON_GETTEXT_FILES) \
bbstreamer_tar.c \
bbstreamer_zstd.c \
pg_basebackup.c \
+ pg_createsubscriber.c \
pg_receivewal.c \
pg_recvlogical.c \
receivelog.c \
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index e24ed7ef506..91c3a2f0036 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -43,7 +43,7 @@ struct CreateSubscriberOptions
SimpleStringList sub_names; /* list of subscription names */
SimpleStringList replslot_names; /* list of replication slot names */
int recovery_timeout; /* stop recovery after this time */
-} CreateSubscriberOptions;
+};
struct LogicalRepInfo
{
@@ -57,7 +57,7 @@ struct LogicalRepInfo
bool made_replslot; /* replication slot was created */
bool made_publication; /* publication was created */
-} LogicalRepInfo;
+};
static void cleanup_objects_atexit(void);
static void usage();
@@ -155,9 +155,9 @@ cleanup_objects_atexit(void)
*/
if (recovery_ended)
{
- pg_log_warning("pg_createsubscriber failed after the end of recovery");
- pg_log_warning_hint("The target server cannot be used as a physical replica anymore.");
- pg_log_warning_hint("You must recreate the physical replica before continuing.");
+ pg_log_warning("failed after the end of recovery");
+ pg_log_warning_hint("The target server cannot be used as a physical replica anymore. "
+ "You must recreate the physical replica before continuing.");
}
for (int i = 0; i < num_dbs; i++)
@@ -184,13 +184,13 @@ cleanup_objects_atexit(void)
*/
if (dbinfo[i].made_publication)
{
- pg_log_warning("There might be a publication \"%s\" in database \"%s\" on primary",
+ pg_log_warning("publication \"%s\" in database \"%s\" on primary might be left behind",
dbinfo[i].pubname, dbinfo[i].dbname);
pg_log_warning_hint("Consider dropping this publication before trying again.");
}
if (dbinfo[i].made_replslot)
{
- pg_log_warning("There might be a replication slot \"%s\" in database \"%s\" on primary",
+ pg_log_warning("replication slot \"%s\" in database \"%s\" on primary might be left behind",
dbinfo[i].replslotname, dbinfo[i].dbname);
pg_log_warning_hint("Drop this replication slot soon to avoid retention of WAL files.");
}
@@ -420,7 +420,7 @@ store_pub_sub_info(const struct CreateSubscriberOptions *opt, const char *pub_ba
SimpleStringListCell *replslotcell = NULL;
int i = 0;
- dbinfo = (struct LogicalRepInfo *) pg_malloc(num_dbs * sizeof(struct LogicalRepInfo));
+ dbinfo = pg_malloc_array(struct LogicalRepInfo, num_dbs);
if (num_pubs > 0)
pubcell = opt->pub_names.head;
@@ -611,7 +611,7 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
char *cmd_str;
- pg_log_info("modifying system identifier from subscriber");
+ pg_log_info("modifying system identifier of subscriber");
cf = get_controlfile(subscriber_dir, &crc_ok);
if (!crc_ok)
@@ -965,7 +965,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo)
/* The target server must be a standby */
if (!server_is_in_recovery(conn))
{
- pg_log_error("The target server must be a standby");
+ pg_log_error("target server must be a standby");
disconnect_database(conn, true);
}
@@ -1217,13 +1217,11 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
{
PQExpBuffer str = createPQExpBuffer();
PGresult *res = NULL;
- char slot_name[NAMEDATALEN];
+ const char *slot_name = dbinfo->replslotname;
char *lsn = NULL;
Assert(conn != NULL);
- snprintf(slot_name, NAMEDATALEN, "%s", dbinfo->replslotname);
-
pg_log_info("creating the replication slot \"%s\" on database \"%s\"",
slot_name, dbinfo->dbname);
@@ -1451,8 +1449,7 @@ wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions
pg_fatal("server did not end recovery");
pg_log_info("target server reached the consistent state");
- pg_log_info_hint("If pg_createsubscriber fails after this point, "
- "you must recreate the physical replica before continuing.");
+ pg_log_info_hint("If pg_createsubscriber fails after this point, you must recreate the physical replica before continuing.");
}
/*
@@ -1625,8 +1622,8 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
PQExpBuffer str = createPQExpBuffer();
PGresult *res;
Oid suboid;
- char originname[NAMEDATALEN];
- char lsnstr[17 + 1]; /* MAXPG_LSNLEN = 17 */
+ char *originname;
+ char *lsnstr;
Assert(conn != NULL);
@@ -1653,13 +1650,12 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
if (dry_run)
{
suboid = InvalidOid;
- snprintf(lsnstr, sizeof(lsnstr), "%X/%X",
- LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
+ lsnstr = psprintf("%X/%X", LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
}
else
{
suboid = strtoul(PQgetvalue(res, 0, 0), NULL, 10);
- snprintf(lsnstr, sizeof(lsnstr), "%s", lsn);
+ lsnstr = psprintf("%s", lsn);
}
PQclear(res);
@@ -1668,7 +1664,7 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
* The origin name is defined as pg_%u. %u is the subscription OID. See
* ApplyWorkerMain().
*/
- snprintf(originname, sizeof(originname), "pg_%u", suboid);
+ originname = psprintf("pg_%u", suboid);
pg_log_info("setting the replication progress (node name \"%s\" ; LSN %s) on database \"%s\"",
originname, lsnstr, dbinfo->dbname);
@@ -1692,6 +1688,8 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
PQclear(res);
}
+ pg_free(originname);
+ pg_free(lsnstr);
destroyPQExpBuffer(str);
}
@@ -1797,13 +1795,9 @@ main(int argc, char **argv)
opt.config_file = NULL;
opt.pub_conninfo_str = NULL;
opt.socket_dir = NULL;
- opt.sub_port = palloc(16);
- strcpy(opt.sub_port, DEFAULT_SUB_PORT);
+ opt.sub_port = DEFAULT_SUB_PORT;
opt.sub_username = NULL;
- opt.database_names = (SimpleStringList)
- {
- NULL, NULL
- };
+ opt.database_names = (SimpleStringList){0};
opt.recovery_timeout = 0;
/*
@@ -1847,7 +1841,6 @@ main(int argc, char **argv)
dry_run = true;
break;
case 'p':
- pg_free(opt.sub_port);
opt.sub_port = pg_strdup(optarg);
break;
case 'P':
base-commit: 48018f1d8c12d42b53c2a855626ee1ceb7f4ca71
prerequisite-patch-id: e4c7223061ca1e12dfa84f4a5ccdc3a9ab0c268a
prerequisite-patch-id: 6cc7583799a18d1119c8ecbb400b4471c6873768
--
2.44.0
Attachments:
[text/plain] 0001-Various-improvements.patch (8.0K, ../[email protected]/2-0001-Various-improvements.patch)
download | inline diff:
From ec8e6ed6c3325a6f9fde2d1632346e212ade9c9f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Mon, 18 Mar 2024 14:48:55 +0100
Subject: [PATCH] Various improvements
---
src/bin/pg_basebackup/Makefile | 2 +-
src/bin/pg_basebackup/nls.mk | 1 +
src/bin/pg_basebackup/pg_createsubscriber.c | 49 +++++++++------------
3 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile
index e9a920dbcda..26c53e473f5 100644
--- a/src/bin/pg_basebackup/Makefile
+++ b/src/bin/pg_basebackup/Makefile
@@ -49,7 +49,7 @@ all: pg_basebackup pg_createsubscriber pg_receivewal pg_recvlogical
pg_basebackup: $(BBOBJS) $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $(BBOBJS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
-pg_createsubscriber: $(WIN32RES) pg_createsubscriber.o | submake-libpq submake-libpgport submake-libpgfeutils
+pg_createsubscriber: pg_createsubscriber.o $(WIN32RES) | submake-libpq submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
pg_receivewal: pg_receivewal.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
diff --git a/src/bin/pg_basebackup/nls.mk b/src/bin/pg_basebackup/nls.mk
index fc475003e8e..7870cea71ce 100644
--- a/src/bin/pg_basebackup/nls.mk
+++ b/src/bin/pg_basebackup/nls.mk
@@ -8,6 +8,7 @@ GETTEXT_FILES = $(FRONTEND_COMMON_GETTEXT_FILES) \
bbstreamer_tar.c \
bbstreamer_zstd.c \
pg_basebackup.c \
+ pg_createsubscriber.c \
pg_receivewal.c \
pg_recvlogical.c \
receivelog.c \
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index e24ed7ef506..91c3a2f0036 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -43,7 +43,7 @@ struct CreateSubscriberOptions
SimpleStringList sub_names; /* list of subscription names */
SimpleStringList replslot_names; /* list of replication slot names */
int recovery_timeout; /* stop recovery after this time */
-} CreateSubscriberOptions;
+};
struct LogicalRepInfo
{
@@ -57,7 +57,7 @@ struct LogicalRepInfo
bool made_replslot; /* replication slot was created */
bool made_publication; /* publication was created */
-} LogicalRepInfo;
+};
static void cleanup_objects_atexit(void);
static void usage();
@@ -155,9 +155,9 @@ cleanup_objects_atexit(void)
*/
if (recovery_ended)
{
- pg_log_warning("pg_createsubscriber failed after the end of recovery");
- pg_log_warning_hint("The target server cannot be used as a physical replica anymore.");
- pg_log_warning_hint("You must recreate the physical replica before continuing.");
+ pg_log_warning("failed after the end of recovery");
+ pg_log_warning_hint("The target server cannot be used as a physical replica anymore. "
+ "You must recreate the physical replica before continuing.");
}
for (int i = 0; i < num_dbs; i++)
@@ -184,13 +184,13 @@ cleanup_objects_atexit(void)
*/
if (dbinfo[i].made_publication)
{
- pg_log_warning("There might be a publication \"%s\" in database \"%s\" on primary",
+ pg_log_warning("publication \"%s\" in database \"%s\" on primary might be left behind",
dbinfo[i].pubname, dbinfo[i].dbname);
pg_log_warning_hint("Consider dropping this publication before trying again.");
}
if (dbinfo[i].made_replslot)
{
- pg_log_warning("There might be a replication slot \"%s\" in database \"%s\" on primary",
+ pg_log_warning("replication slot \"%s\" in database \"%s\" on primary might be left behind",
dbinfo[i].replslotname, dbinfo[i].dbname);
pg_log_warning_hint("Drop this replication slot soon to avoid retention of WAL files.");
}
@@ -420,7 +420,7 @@ store_pub_sub_info(const struct CreateSubscriberOptions *opt, const char *pub_ba
SimpleStringListCell *replslotcell = NULL;
int i = 0;
- dbinfo = (struct LogicalRepInfo *) pg_malloc(num_dbs * sizeof(struct LogicalRepInfo));
+ dbinfo = pg_malloc_array(struct LogicalRepInfo, num_dbs);
if (num_pubs > 0)
pubcell = opt->pub_names.head;
@@ -611,7 +611,7 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
char *cmd_str;
- pg_log_info("modifying system identifier from subscriber");
+ pg_log_info("modifying system identifier of subscriber");
cf = get_controlfile(subscriber_dir, &crc_ok);
if (!crc_ok)
@@ -965,7 +965,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo)
/* The target server must be a standby */
if (!server_is_in_recovery(conn))
{
- pg_log_error("The target server must be a standby");
+ pg_log_error("target server must be a standby");
disconnect_database(conn, true);
}
@@ -1217,13 +1217,11 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
{
PQExpBuffer str = createPQExpBuffer();
PGresult *res = NULL;
- char slot_name[NAMEDATALEN];
+ const char *slot_name = dbinfo->replslotname;
char *lsn = NULL;
Assert(conn != NULL);
- snprintf(slot_name, NAMEDATALEN, "%s", dbinfo->replslotname);
-
pg_log_info("creating the replication slot \"%s\" on database \"%s\"",
slot_name, dbinfo->dbname);
@@ -1451,8 +1449,7 @@ wait_for_end_recovery(const char *conninfo, const struct CreateSubscriberOptions
pg_fatal("server did not end recovery");
pg_log_info("target server reached the consistent state");
- pg_log_info_hint("If pg_createsubscriber fails after this point, "
- "you must recreate the physical replica before continuing.");
+ pg_log_info_hint("If pg_createsubscriber fails after this point, you must recreate the physical replica before continuing.");
}
/*
@@ -1625,8 +1622,8 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
PQExpBuffer str = createPQExpBuffer();
PGresult *res;
Oid suboid;
- char originname[NAMEDATALEN];
- char lsnstr[17 + 1]; /* MAXPG_LSNLEN = 17 */
+ char *originname;
+ char *lsnstr;
Assert(conn != NULL);
@@ -1653,13 +1650,12 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
if (dry_run)
{
suboid = InvalidOid;
- snprintf(lsnstr, sizeof(lsnstr), "%X/%X",
- LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
+ lsnstr = psprintf("%X/%X", LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
}
else
{
suboid = strtoul(PQgetvalue(res, 0, 0), NULL, 10);
- snprintf(lsnstr, sizeof(lsnstr), "%s", lsn);
+ lsnstr = psprintf("%s", lsn);
}
PQclear(res);
@@ -1668,7 +1664,7 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
* The origin name is defined as pg_%u. %u is the subscription OID. See
* ApplyWorkerMain().
*/
- snprintf(originname, sizeof(originname), "pg_%u", suboid);
+ originname = psprintf("pg_%u", suboid);
pg_log_info("setting the replication progress (node name \"%s\" ; LSN %s) on database \"%s\"",
originname, lsnstr, dbinfo->dbname);
@@ -1692,6 +1688,8 @@ set_replication_progress(PGconn *conn, const struct LogicalRepInfo *dbinfo, cons
PQclear(res);
}
+ pg_free(originname);
+ pg_free(lsnstr);
destroyPQExpBuffer(str);
}
@@ -1797,13 +1795,9 @@ main(int argc, char **argv)
opt.config_file = NULL;
opt.pub_conninfo_str = NULL;
opt.socket_dir = NULL;
- opt.sub_port = palloc(16);
- strcpy(opt.sub_port, DEFAULT_SUB_PORT);
+ opt.sub_port = DEFAULT_SUB_PORT;
opt.sub_username = NULL;
- opt.database_names = (SimpleStringList)
- {
- NULL, NULL
- };
+ opt.database_names = (SimpleStringList){0};
opt.recovery_timeout = 0;
/*
@@ -1847,7 +1841,6 @@ main(int argc, char **argv)
dry_run = true;
break;
case 'p':
- pg_free(opt.sub_port);
opt.sub_port = pg_strdup(optarg);
break;
case 'P':
base-commit: 48018f1d8c12d42b53c2a855626ee1ceb7f4ca71
prerequisite-patch-id: e4c7223061ca1e12dfa84f4a5ccdc3a9ab0c268a
prerequisite-patch-id: 6cc7583799a18d1119c8ecbb400b4471c6873768
--
2.44.0
view thread (107+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: speed up a logical replica setup
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox