Dear Hayato,
Thank you for the review! My apologies for the error in the patch -- it
looks like I accidentally modified it before sending =(. I've attached the
fixed versions below.
> Regarding patch content, your patch restores the postgresql.auto.conf
after the
> command runs. Initially I felt that it is enough to set below GUCs
becasue only
> they are changed from the default. Is there a reason why you fully
restore them?
I just found it easier to restore the original state of
'postgresql.auto.conf', as removing parameters from the file resets them to
their default values. This approach achieves the same final state without
having to explicitly set each one.
From baed4d6390737c4274cd4dc1c21d7bf488c673d4 Mon Sep 17 00:00:00 2001
From: Alena Vinter <dlaar...@gmail.com>
Date: Tue, 2 Sep 2025 18:15:13 +0700
Subject: [PATCH 2/2] Implements helper function in recovery_gen
These functions support pg_createsubscriber's need to temporarily
configure recovery params and ensure proper cleanup after the conversion
to logical replication is complete.
---
src/fe_utils/recovery_gen.c | 77 +++++++++++++++++++++++++++++
src/include/fe_utils/recovery_gen.h | 3 ++
2 files changed, 80 insertions(+)
diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c
index e9023584768..e8e0dde9e00 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -10,6 +10,7 @@
#include "postgres_fe.h"
#include "common/logging.h"
+#include "common/file_utils.h"
#include "fe_utils/recovery_gen.h"
#include "fe_utils/string_utils.h"
@@ -234,3 +235,79 @@ GetDbnameFromConnectionOptions(const char *connstr)
PQconninfoFree(conn_opts);
return dbname;
}
+
+PQExpBuffer
+GetRecoveryConfig(PGconn *pgconn, const char *target_dir)
+{
+ PQExpBuffer contents;
+ char filename[MAXPGPATH];
+ FILE *cf;
+ bool use_recovery_conf;
+
+ char data[1024];
+ size_t bytes_read;
+
+ Assert(pgconn != NULL);
+
+ contents = createPQExpBuffer();
+ if (!contents)
+ pg_fatal("out of memory");
+
+ use_recovery_conf =
+ PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_RECOVERY_GUC;
+
+ snprintf(filename, MAXPGPATH, "%s/%s", target_dir,
+ use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf");
+
+ cf = fopen(filename, "r");
+ if (cf == NULL)
+ pg_fatal("could not open file \"%s\": %m", filename);
+
+ while ((bytes_read = fread(data, 1, sizeof(data), cf)) > 0)
+ {
+ data[bytes_read] = '\0';
+ appendPQExpBufferStr(contents, data);
+ }
+
+ if (ferror(cf))
+ {
+ pg_fatal("could not read from file \"%s\": %m", filename);
+ }
+
+ fclose(cf);
+
+ return contents;
+}
+
+void
+ReplaceRecoveryConfig(PGconn *pgconn, const char *target_dir, PQExpBuffer contents)
+{
+ char tmp_filename[MAXPGPATH];
+ char filename[MAXPGPATH];
+ FILE *cf;
+ bool use_recovery_conf;
+
+ Assert(pgconn != NULL);
+
+ use_recovery_conf =
+ PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_RECOVERY_GUC;
+
+ snprintf(tmp_filename, MAXPGPATH, "%s/%s.tmp", target_dir,
+ use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf");
+
+ snprintf(filename, MAXPGPATH, "%s/%s", target_dir,
+ use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf");
+
+ cf = fopen(tmp_filename, "w");
+ if (cf == NULL)
+ pg_fatal("could not open file \"%s\": %m", tmp_filename);
+
+ if (fwrite(contents->data, contents->len, 1, cf) != 1)
+ pg_fatal("could not write to file \"%s\": %m", tmp_filename);
+
+ fclose(cf);
+
+ if (durable_rename(tmp_filename, filename) != 0)
+ pg_fatal("could not rename file \"%s\" to \"%s\": %m",
+ tmp_filename, filename);
+}
diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h
index c13f2263bcd..18219af966b 100644
--- a/src/include/fe_utils/recovery_gen.h
+++ b/src/include/fe_utils/recovery_gen.h
@@ -27,4 +27,7 @@ extern void WriteRecoveryConfig(PGconn *pgconn, const char *target_dir,
PQExpBuffer contents);
extern char *GetDbnameFromConnectionOptions(const char *connstr);
+extern PQExpBuffer GetRecoveryConfig(PGconn *pgconn, const char *target_dir);
+extern void ReplaceRecoveryConfig(PGconn *pgconn, const char *target_dir, PQExpBuffer contents);
+
#endif /* RECOVERY_GEN_H */
--
2.51.0
From 94ca21fd0b3e683c4b03b552a3c7baa7f376eed3 Mon Sep 17 00:00:00 2001
From: Alena Vinter <dlaar...@gmail.com>
Date: Tue, 15 Jul 2025 15:21:22 +0700
Subject: [PATCH 1/2] Reseting recovery target parameters in
pg_createsubscriber
The utility sets recovery target params for correct recovery before
conversion a physical replica to a logical one but does not reset them
afterward. It may cause recovery failures in certain scenarios.
For example, if recovery begins from a checkpoint where no WAL records
need to be applied, the system may incorrectly determine that the
recovery target was never reached because these parameters remain
active.
This change ensures all recovery parameters are properly reset after
conversion to prevent such edge cases.
---
src/bin/pg_basebackup/pg_createsubscriber.c | 127 +++++++++++++-----
.../t/040_pg_createsubscriber.pl | 17 +++
2 files changed, 113 insertions(+), 31 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 1d0fe44b6d3..8ddb56c641e 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -99,6 +99,8 @@ static void setup_subscriber(struct LogicalRepInfo *dbinfo,
const char *consistent_lsn);
static void setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir,
const char *lsn);
+static void reset_recovery_params(const struct LogicalRepInfo *dbinfo,
+ const char *datadir);
static void drop_primary_replication_slot(struct LogicalRepInfo *dbinfo,
const char *slotname);
static void drop_failover_replication_slots(struct LogicalRepInfo *dbinfo);
@@ -161,6 +163,43 @@ enum WaitPMResult
POSTMASTER_STILL_STARTING
};
+typedef struct RecoveryParam {
+ const char *name;
+ const char *value;
+} RecoveryParam;
+
+/*
+ * Recovery parameters to be configured during physical replication setup.
+ * Most parameters are initialized, except recovery_target_lsn which is set
+ * separately during setup_recovery().
+ */
+static const RecoveryParam recovery_params[] = {
+ {"recovery_target", "''"},
+ {"recovery_target_timeline", "'latest'"},
+ /*
+ * Set recovery_target_inclusive = false to avoid reapplying the
+ * transaction committed at 'lsn' after subscription is enabled. This is
+ * because the provided 'lsn' is also used as the replication start point
+ * for the subscription. So, the server can send the transaction committed
+ * at that 'lsn' after replication is started which can lead to applying
+ * the same transaction twice if we keep recovery_target_inclusive = true.
+ */
+ {"recovery_target_inclusive", "false"},
+ {"recovery_target_action", "promote"},
+ {"recovery_target_name", "''"},
+ {"recovery_target_time", "''"},
+ {"recovery_target_xid", "''"},
+ {"recovery_target_lsn", NULL}, /* the value will be specified later in setup_recovery*/
+ {NULL, NULL}, /* sentinel */
+};
+
+/*
+ * Buffer to preserve the original recovery conf contents before modifying
+ * recovery parameters. This allows restoration of the original configuration
+ * after the logical replication process completes, maintaining the system's
+ * previous recovery state.
+ */
+static PQExpBuffer recoveryconfcontents;
/*
* Cleanup objects that were created by pg_createsubscriber if there is an
@@ -1227,7 +1266,7 @@ static void
setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const char *lsn)
{
PGconn *conn;
- PQExpBuffer recoveryconfcontents;
+ PQExpBuffer generatedrecoveryconfcontents;
/*
* Despite of the recovery parameters will be written to the subscriber,
@@ -1236,6 +1275,9 @@ setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const c
*/
conn = connect_database(dbinfo[0].pubconninfo, true);
+ /* Before setting up the recovery parameters save the original content. */
+ recoveryconfcontents = GetRecoveryConfig(conn, datadir);
+
/*
* Write recovery parameters.
*
@@ -1246,43 +1288,63 @@ setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const c
* state is reached (recovery_target) and failure due to multiple recovery
* targets (name, time, xid, LSN).
*/
- recoveryconfcontents = GenerateRecoveryConfig(conn, NULL, NULL);
- appendPQExpBufferStr(recoveryconfcontents, "recovery_target = ''\n");
- appendPQExpBufferStr(recoveryconfcontents,
- "recovery_target_timeline = 'latest'\n");
+ generatedrecoveryconfcontents = GenerateRecoveryConfig(conn, NULL, NULL);
- /*
- * Set recovery_target_inclusive = false to avoid reapplying the
- * transaction committed at 'lsn' after subscription is enabled. This is
- * because the provided 'lsn' is also used as the replication start point
- * for the subscription. So, the server can send the transaction committed
- * at that 'lsn' after replication is started which can lead to applying
- * the same transaction twice if we keep recovery_target_inclusive = true.
- */
- appendPQExpBufferStr(recoveryconfcontents,
- "recovery_target_inclusive = false\n");
- appendPQExpBufferStr(recoveryconfcontents,
- "recovery_target_action = promote\n");
- appendPQExpBufferStr(recoveryconfcontents, "recovery_target_name = ''\n");
- appendPQExpBufferStr(recoveryconfcontents, "recovery_target_time = ''\n");
- appendPQExpBufferStr(recoveryconfcontents, "recovery_target_xid = ''\n");
+ for (int i = 0; recovery_params[i].name != NULL; i++)
+ {
+ if (strcmp(recovery_params[i].name, "recovery_target_lsn") == 0)
+ {
+ const char *lsn_str;
+
+ if (dry_run)
+ lsn_str = psprintf("%X/%X", LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
+ else
+ lsn_str = lsn;
+
+ appendPQExpBuffer(generatedrecoveryconfcontents, "%s = '%s'\n",
+ recovery_params[i].name, lsn_str);
+ }
+ else
+ {
+ appendPQExpBuffer(generatedrecoveryconfcontents, "%s = %s\n",
+ recovery_params[i].name, recovery_params[i].value);
+ }
+ }
+
+ if (dry_run)
+ appendPQExpBufferStr(generatedrecoveryconfcontents, "# dry run mode");
+ else
+ WriteRecoveryConfig(conn, datadir, generatedrecoveryconfcontents);
+
+ disconnect_database(conn, false);
+
+ pg_log_debug("recovery parameters:\n%s", generatedrecoveryconfcontents->data);
+}
+
+/*
+ * Reset the previously set recovery parameters.
+ */
+static void
+reset_recovery_params(const struct LogicalRepInfo *dbinfo, const char *datadir)
+{
+ PGconn *conn;
+ PQExpBuffer generatedrecoveryconfcontents;
+
+ conn = connect_database(dbinfo[0].pubconninfo, true);
+
+ generatedrecoveryconfcontents = GenerateRecoveryConfig(conn, NULL, NULL);
+
+ appendPQExpBuffer(recoveryconfcontents, "%s",
+ generatedrecoveryconfcontents->data);
if (dry_run)
- {
appendPQExpBufferStr(recoveryconfcontents, "# dry run mode");
- appendPQExpBuffer(recoveryconfcontents,
- "recovery_target_lsn = '%X/%X'\n",
- LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr));
- }
else
- {
- appendPQExpBuffer(recoveryconfcontents, "recovery_target_lsn = '%s'\n",
- lsn);
- WriteRecoveryConfig(conn, datadir, recoveryconfcontents);
- }
+ ReplaceRecoveryConfig(conn, datadir, recoveryconfcontents);
+
disconnect_database(conn, false);
- pg_log_debug("recovery parameters:\n%s", recoveryconfcontents->data);
+ pg_log_debug("recovery parameters were reset");
}
/*
@@ -2458,6 +2520,9 @@ main(int argc, char **argv)
pg_log_info("stopping the subscriber");
stop_standby_server(subscriber_dir);
+ /* Reset recovery parameters */
+ reset_recovery_params(dbinfos.dbinfo, subscriber_dir);
+
/* Change system identifier from subscriber */
modify_subscriber_sysid(&opt);
diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 229fef5b3b5..69fb6a3dbef 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -41,6 +41,16 @@ sub generate_db
return $dbname;
}
+sub test_param_absent {
+ my ($node, $param) = @_;
+ my $auto_conf = $node->data_dir . '/postgresql.auto.conf';
+
+ return 1 unless -e $auto_conf;
+
+ my $content = slurp_file($auto_conf);
+ return $content !~ /^\s*$param\s*=/m;
+}
+
#
# Test mandatory options
command_fails(['pg_createsubscriber'],
@@ -467,6 +477,13 @@ command_ok(
],
'run pg_createsubscriber on node S');
+# Verify that recovery parameters have been reset after pg_createsubscriber
+# We check recovery_target_lsn as a representative parameter - since all
+# recovery parameters are managed as a group, the absence of one indicates
+# that the entire set has been properly cleared from the configuration.
+ok(test_param_absent($node_s, 'recovery_target_lsn'),
+ 'recovery_target_lsn parameter was removed');
+
# Confirm the physical replication slot has been removed
$result = $node_p->safe_psql($db1,
"SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$slotname'"
--
2.51.0
