Sorry, wrong patches again. Here are the correct ones.
Best regards,
Alyona Vinter
From baed4d6390737c4274cd4dc1c21d7bf488c673d4 Mon Sep 17 00:00:00 2001
From: Alena Vinter <[email protected]>
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 <[email protected]>
Date: Tue, 2 Sep 2025 18:15:13 +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..b34e840b254 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/%08X'\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
