From ce69fdbb0c2f7ce340ac714f2aea8e0f1072069a Mon Sep 17 00:00:00 2001 From: Alena Vinter 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