Hi, I'm back with improvements :) I've added code comments in `recovery_gen.c` and expanded the documentation in `pg_createsubscriber.sgml`.
About the recovery parameters cleanup: I thought about adding an exit callback, but it doesn't really make sense because once the target server gets promoted (which happens soon after we set the parameters), there's no point in cleaning up - the server is already promoted and can't be used as a replica again and must be recreated. Also, `reset_recovery_params()` might call `exit()` itself, which could cause problems with the cleanup callback. So I think it's better to just warn users about leftover parameters and let them handle the cleanup manually if needed. By the way, is it ok that the second patch includes both code and test changes together, or should I split them into separate commits? I look forward to your feedback! Regards, Alena Vinter
From ce69fdbb0c2f7ce340ac714f2aea8e0f1072069a 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 | 59 ++++++++++++++++++- .../t/040_pg_createsubscriber.pl | 18 ++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index 3986882f042..7ff2708bfff 100644 --- a/src/bin/pg_basebackup/pg_createsubscriber.c +++ b/src/bin/pg_basebackup/pg_createsubscriber.c @@ -154,6 +154,7 @@ static char *subscriber_dir = NULL; static bool recovery_ended = false; static bool standby_running = false; +static bool recovery_params_set = false; enum WaitPMResult { @@ -161,6 +162,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 +177,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) @@ -190,6 +197,18 @@ cleanup_objects_atexit(void) pg_log_warning_hint("The target server cannot be used as a physical replica anymore. " "You must recreate the physical replica before continuing."); } + else if (recovery_params_set) + { + /* + * Skip parameter reset check since reset happens after recovery. + * Therefore, leftover parameters don't matter as the target needs + * full recreation. + */ + 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++) { @@ -1236,6 +1255,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 +1300,9 @@ 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 +1310,31 @@ 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); + 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 +2508,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
From 5480fe56ec6b7f51797c2a9566dc555243ad9549 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 | 101 ++++++++++++++++++++++++++++ src/include/fe_utils/recovery_gen.h | 3 + 2 files changed, 104 insertions(+) diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c index e9023584768..f5e9e968863 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,103 @@ 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. + */ +void +ReplaceRecoveryConfig(PGconn *pgconn, const char *target_dir, PQExpBuffer contents) +{ + char tmp_filename[MAXPGPATH]; + char filename[MAXPGPATH]; + FILE *cf; + const char *config_filename = + (PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_RECOVERY_GUC) + ? "recovery.conf" + : "postgresql.auto.conf"; + + Assert(pgconn != NULL); + + /* + * 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_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); + + /* + * 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_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 068ee56f7a84b272098b2402501db48b1405ce31 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..bf6e1844a18 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 may 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
