Hi, > In short, I disagree with what you are doing here: we should take the > extra step and clean up anything that's been created by the tool when > we know we can safely do so
I got your point, thanks for pointing to the `pg_rewind` case. I've attached a new version of the patches. I've changed `ReplaceRecoveryConfig` a little bit -- now it returns false in case of an error instead of exiting. Best wishes, Alena Vinter
From d9f96c1b5ea468ac4dea427e51d9beb3cd087653 Mon Sep 17 00:00:00 2001 From: Alena Vinter <[email protected]> Date: Tue, 2 Sep 2025 18:15:13 +0700 Subject: [PATCH 1/3] 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 | 117 ++++++++++++++++++++++++++++ src/include/fe_utils/recovery_gen.h | 4 + 2 files changed, 121 insertions(+) diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c index e9023584768..e22d7673ff8 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,119 @@ GetDbnameFromConnectionOptions(const char *connstr) PQconninfoFree(conn_opts); return dbname; } + +/* + * GetRecoveryConfig + * + * Reads the recovery configuration file from the target server's data directory + * and returns its contents as a PQExpBuffer. + */ +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); + + /* Read file contents in chunks and append to the buffer */ + 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; +} + +/* + * ReplaceRecoveryConfig + * + * Replaces the recovery configuration file on the target server with new contents. + * + * The operation is performed atomically by writing to a temporary file first, + * then renaming it to the final filename. + * + * Returns false if an error occurs. In case of error, a temporary file may + * still be present on the server. + */ +bool +ReplaceRecoveryConfig(PGconn *pgconn, const char *target_dir, PQExpBuffer contents) +{ + char tmp_filename[MAXPGPATH]; + char filename[MAXPGPATH]; + FILE *cf; + const char *config_filename; + + Assert(pgconn != NULL); + + config_filename = + (PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_RECOVERY_GUC) + ? "recovery.conf" + : "postgresql.auto.conf"; + + /* + * Construct full paths for the configuration file and its temporary + * version + */ + snprintf(filename, MAXPGPATH, "%s/%s", target_dir, config_filename); + snprintf(tmp_filename, MAXPGPATH, "%s.tmp", filename); + + /* + * Open temporary file for writing. Mode "w" ensures the file is recreated + * if it already exists. + */ + cf = fopen(tmp_filename, "w"); + if (cf == NULL) + { + pg_log_error("could not open file \"%s\": %m", tmp_filename); + return false; + } + + if (fwrite(contents->data, contents->len, 1, cf) != 1) + { + pg_log_error("could not write to file \"%s\": %m", tmp_filename); + return false; + } + + fclose(cf); + + /* + * Atomically replace the old configuration file with the new one by + * renaming the temporary file to the final filename. + */ + if (durable_rename(tmp_filename, filename) != 0) + { + pg_log_error("could not rename file \"%s\" to \"%s\": %m", + tmp_filename, filename); + return false; + } + + return true; +} diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h index c13f2263bcd..f5f21ec088d 100644 --- a/src/include/fe_utils/recovery_gen.h +++ b/src/include/fe_utils/recovery_gen.h @@ -27,4 +27,8 @@ 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 bool ReplaceRecoveryConfig(PGconn *pgconn, const char *target_dir, + PQExpBuffer contents); + #endif /* RECOVERY_GEN_H */ -- 2.51.0
From e3195ccf67312fa39c2751b9d021760e5eb1ddfd Mon Sep 17 00:00:00 2001 From: Alena Vinter <[email protected]> Date: Tue, 23 Sep 2025 10:23:46 +0700 Subject: [PATCH 3/3] doc: Add warning about leftover recovery parameters in pg_createsubscriber --- doc/src/sgml/ref/pg_createsubscriber.sgml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml b/doc/src/sgml/ref/pg_createsubscriber.sgml index bb9cc72576c..fa14aeeef13 100644 --- a/doc/src/sgml/ref/pg_createsubscriber.sgml +++ b/doc/src/sgml/ref/pg_createsubscriber.sgml @@ -520,11 +520,19 @@ PostgreSQL documentation are added to avoid unexpected behavior during the recovery process such as end of the recovery as soon as a consistent state is reached (WAL should be applied until the replication start location) and multiple - recovery targets that can cause a failure. This step finishes once the - server ends standby mode and is accepting read-write transactions. If - <option>--recovery-timeout</option> option is set, - <application>pg_createsubscriber</application> terminates if recovery - does not end until the given number of seconds. + recovery targets that can cause a failure. + </para> + <para> + If an error occurs during this step, recovery parameters might remain in + the target server's configuration. You may need to manually remove these + leftover parameters before attempting to run. + <application>pg_createsubscriber</application> again. + </para> + <para> + This step finishes once the server ends standby mode and is accepting + read-write transactions. If <option>--recovery-timeout</option> option + is set, <application>pg_createsubscriber</application> terminates if + recovery does not end until the given number of seconds. </para> </step> -- 2.51.0
From 7544cc434c488caf834e812be036caac5d9493b5 Mon Sep 17 00:00:00 2001 From: Alena Vinter <[email protected]> Date: Tue, 2 Sep 2025 18:15:13 +0700 Subject: [PATCH 2/3] 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 | 73 ++++++++++++++++++- .../t/040_pg_createsubscriber.pl | 18 +++++ 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index 3986882f042..714e8659dcb 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -154,6 +154,8 @@ static char *subscriber_dir = NULL; static bool recovery_ended = false; static bool standby_running = false; +static bool recovery_params_set = false; +static bool recovery_params_reset = false; enum WaitPMResult { @@ -161,6 +163,13 @@ enum WaitPMResult POSTMASTER_STILL_STARTING }; +/* + * 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 savedrecoveryconfcontents = NULL; /* * Cleanup objects that were created by pg_createsubscriber if there is an @@ -169,9 +178,8 @@ enum WaitPMResult * Publications and replication slots are created on primary. Depending on the * step it failed, it should remove the already created objects if it is * possible (sometimes it won't work due to a connection issue). - * There is no cleanup on the target server. The steps on the target server are - * executed *after* promotion, hence, at this point, a failure means recreate - * the physical replica and start again. + * There is no cleanup on the target server *after* its promotion because any + * failure at this point means recreate the physical replica and start again. */ static void cleanup_objects_atexit(void) @@ -191,6 +199,27 @@ cleanup_objects_atexit(void) "You must recreate the physical replica before continuing."); } + if (recovery_params_set && !recovery_params_reset) + { + PGconn *conn; + bool no_err = true; + + conn = connect_database(dbinfos.dbinfo[0].pubconninfo, false); + if (conn != NULL) + { + no_err = ReplaceRecoveryConfig(conn, subscriber_dir, + savedrecoveryconfcontents); + disconnect_database(conn, false); + pg_log_info("previously set recovery parameters were properly reset on the target"); + } + if (conn == NULL || !no_err) + { + pg_log_warning("recovery parameters were set but not properly reset on the target"); + pg_log_warning_hint("Manual removal of recovery parameters is required from 'postgresql.auto.conf' (PostgreSQL %d+) or 'recovery.conf' (older versions)", + MINIMUM_VERSION_FOR_RECOVERY_GUC / 10000); + } + } + for (int i = 0; i < num_dbs; i++) { struct LogicalRepInfo *dbinfo = &dbinfos.dbinfo[i]; @@ -1236,6 +1265,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. */ + savedrecoveryconfcontents = GetRecoveryConfig(conn, datadir); + /* * Write recovery parameters. * @@ -1278,6 +1310,8 @@ setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const c { appendPQExpBuffer(recoveryconfcontents, "recovery_target_lsn = '%s'\n", lsn); + + recovery_params_set = true; WriteRecoveryConfig(conn, datadir, recoveryconfcontents); } disconnect_database(conn, false); @@ -1285,6 +1319,36 @@ setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const c pg_log_debug("recovery parameters:\n%s", recoveryconfcontents->data); } +/* + * Reset the previously set recovery parameters. + */ +static void +reset_recovery_params(const struct LogicalRepInfo *dbinfo, const char *datadir) +{ + PGconn *conn; + PQExpBuffer recoveryconfcontents; + + conn = connect_database(dbinfo[0].pubconninfo, true); + + recoveryconfcontents = GenerateRecoveryConfig(conn, NULL, NULL); + + appendPQExpBuffer(savedrecoveryconfcontents, "%s", + recoveryconfcontents->data); + + if (dry_run) + { + appendPQExpBufferStr(savedrecoveryconfcontents, "# dry run mode"); + } + else + { + ReplaceRecoveryConfig(conn, datadir, savedrecoveryconfcontents); + recovery_params_reset = true; + } + disconnect_database(conn, false); + + pg_log_debug("recovery parameters were reset"); +} + /* * Drop physical replication slot on primary if the standby was using it. After * the transformation, it has no use. @@ -2458,6 +2522,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..099f1553a5f 100644 --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl @@ -41,6 +41,17 @@ 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 +478,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
