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 <pe...@eisentraut.org>
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
