Hi everyone, Thank you for all the valuable feedback! I've improved the patches in the latest version.
> Based on the contents of the latest patch, we reset the parameters > after promoting the node, and primary_conninfo only matters while we > are in recovery, for a standby recovery WAL using the streaming > replication protocol. Michael, thanks for helping! This fact simplifies the code. I put resetting the parameters exclusively in the `atexit` callback -- this approach seems neater to me. What do you think? > Did I get the idea right? Ian, yes, you got it right. The core issue occurs when postgres encounters a checkpoint during recovery, determines redo isn't needed (because there are no records after the checkpoint), but then fails with a fatal error because it cannot reach the specified LSN target (which is lower than the checkpoint LSN). I reckon this is a recovery logic issue, but I also believe the component that sets recovery parameters should be responsible for cleaning them up when they're no longer required. Best wishes, Alena Vinter
From 6df46b0c9e7accd468946a891d988ad71b7121eb 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 ac319b445cf501fe0349590d9a4d2225a419d660 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 | 56 ++++++++++++++----- .../t/040_pg_createsubscriber.pl | 31 ++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c index 3986882f042..dccf5dcbc87 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,21 +162,51 @@ 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; + +/* + * Cached version of the subscriber server, used for version-specific behavior + */ +static int sub_version = 0; /* - * Cleanup objects that were created by pg_createsubscriber if there is an - * error. + * Cleanup objects that were created by pg_createsubscriber. * * 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) { + /* + * Replace recovery config with its original contents. This ensures the + * subscriber doesn't try using outdated recovery parameters. + */ + if (recovery_params_set) + { + bool no_err = ReplaceRecoveryConfig(sub_version, subscriber_dir, + savedrecoveryconfcontents); + + if (!no_err) + { + bool use_recovery_conf = + sub_version < MINIMUM_VERSION_FOR_RECOVERY_GUC; + + 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 '%s'", + use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf"); + } + } + if (success) return; @@ -1019,6 +1050,7 @@ check_subscriber(const struct LogicalRepInfo *dbinfo) pg_log_info("checking settings on subscriber"); conn = connect_database(dbinfo[0].subconninfo, true); + sub_version = PQserverVersion(conn); /* The target server must be a standby */ if (!server_is_in_recovery(conn)) @@ -1236,6 +1268,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. * @@ -1267,17 +1302,12 @@ setup_recovery(const struct LogicalRepInfo *dbinfo, const char *datadir, const c appendPQExpBufferStr(recoveryconfcontents, "recovery_target_time = ''\n"); appendPQExpBufferStr(recoveryconfcontents, "recovery_target_xid = ''\n"); - if (dry_run) - { - appendPQExpBufferStr(recoveryconfcontents, "# dry run mode"); - appendPQExpBuffer(recoveryconfcontents, - "recovery_target_lsn = '%X/%08X'\n", - LSN_FORMAT_ARGS((XLogRecPtr) InvalidXLogRecPtr)); - } - else + if (!dry_run) { appendPQExpBuffer(recoveryconfcontents, "recovery_target_lsn = '%s'\n", lsn); + + recovery_params_set = true; WriteRecoveryConfig(conn, datadir, recoveryconfcontents); } disconnect_database(conn, false); diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl index 229fef5b3b5..6dee0837ee9 100644 --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl @@ -41,6 +41,30 @@ sub generate_db return $dbname; } +# Check if a parameter is absent from the server's configuration file. +# Uses 'recovery.conf' for PostgreSQL < 12, 'postgresql.auto.conf' for newer +# versions. +sub test_param_absent +{ + my ($node, $param) = @_; + my $version = $node->pg_version->major; + my $conf; + + if ($version < 12) + { + $conf = $node->data_dir . '/recovery.conf'; + } + else + { + $conf = $node->data_dir . '/postgresql.auto.conf'; + } + + return 1 unless -e $conf; + + my $content = slurp_file($conf); + return $content !~ /^\s*$param\s*=/m; +} + # # Test mandatory options command_fails(['pg_createsubscriber'], @@ -467,6 +491,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 29d3f5f7abc730112e8770d3da84232af4754c46 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 | 113 ++++++++++++++++++++++++++++ src/include/fe_utils/recovery_gen.h | 4 + 2 files changed, 117 insertions(+) diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c index e9023584768..e2e501ce693 100644 --- a/src/fe_utils/recovery_gen.c +++ b/src/fe_utils/recovery_gen.c @@ -9,7 +9,9 @@ */ #include "postgres_fe.h" +#include "common/file_utils.h" #include "common/logging.h" +#include "common/string.h" #include "fe_utils/recovery_gen.h" #include "fe_utils/string_utils.h" @@ -234,3 +236,114 @@ 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; + char *line; + bool use_recovery_conf; + + 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 line-by-line and append to the buffer */ + while ((line = pg_get_line(cf, NULL)) != NULL) + { + appendPQExpBufferStr(contents, line); + } + + 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(int version, const char *target_dir, PQExpBuffer contents) +{ + char tmp_filename[MAXPGPATH]; + char filename[MAXPGPATH]; + FILE *cf; + const char *config_filename; + + config_filename = + (version < 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..b54397db740 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(int version, const char *target_dir, + PQExpBuffer contents); + #endif /* RECOVERY_GEN_H */ -- 2.51.0
