From 7593ce45ffc014f01158092c4cfd76e7a7ff8835 Mon Sep 17 00:00:00 2001 From: JoongHyuk Shin Date: Sun, 28 Jun 2026 18:25:45 +0900 Subject: [PATCH v10 2/2] Avoid ERROR in recovery target GUC assign hooks Recovery target parameters are postmaster-startup GUCs, but their assign hooks previously did more than assign individual parameter values. They also updated the global recoveryTarget state and raised ERROR if more than one recovery target appeared to be set. This was not a good fit for GUC assign hooks. Assign hooks should not throw ERROR, and deriving cross-parameter state while individual GUCs are still being assigned makes the result depend on assignment order rather than the final configuration. For example, setting one recovery target and then setting another recovery_target_* parameter to an empty string could clear recoveryTarget, causing recovery to proceed with no target even though a valid target remained configured. Fix this by having the assign hooks only store their own parameter values. The effective recoveryTarget is now derived once from the final recovery_target* settings in validateRecoveryParameters(), which also rejects configurations that specify more than one recovery target with FATAL. This preserves the expected behavior for repeated assignments of the same GUC, treats empty values as "not set", and removes cross-GUC validation from the assign hooks. --- src/backend/access/transam/xlogrecovery.c | 140 +++++++------------- src/backend/utils/misc/guc_parameters.dat | 5 +- src/backend/utils/misc/guc_tables.c | 1 - src/include/access/xlogrecovery.h | 2 +- src/include/utils/guc_hooks.h | 3 - src/test/recovery/t/003_recovery_targets.pl | 123 +++++++++++++---- 6 files changed, 145 insertions(+), 129 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index a9ebac2d0ef..5f3b065b894 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -61,6 +61,7 @@ #include "storage/subsystems.h" #include "utils/datetime.h" #include "utils/fmgrprotos.h" +#include "utils/guc.h" #include "utils/guc_hooks.h" #include "utils/pgstat_internal.h" #include "utils/pg_lsn.h" @@ -92,7 +93,7 @@ int recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE; TransactionId recoveryTargetXid; char *recovery_target_time_string; TimestampTz recoveryTargetTime; -const char *recoveryTargetName; +char *recoveryTargetName; XLogRecPtr recoveryTargetLSN; int recovery_min_apply_delay = 0; @@ -392,6 +393,7 @@ static bool HotStandbyActiveInReplay(void); static void SetCurrentChunkStartTime(TimestampTz xtime); static void SetLatestXTime(TimestampTz xtime); +static RecoveryTargetType DetermineRecoveryTargetType(void); /* * Register shared memory for WAL recovery @@ -1067,6 +1069,9 @@ readRecoverySignalFile(void) static void validateRecoveryParameters(void) { + /* Reject conflicting targets even when recovery was not requested */ + recoveryTarget = DetermineRecoveryTargetType(); + if (!ArchiveRecoveryRequested) return; @@ -4769,30 +4774,50 @@ check_primary_slot_name(char **newval, void **extra, GucSource source) } /* - * Recovery target settings: Only one of the several recovery_target* settings - * may be set. Setting a second one results in an error. The global variable - * recoveryTarget tracks which kind of recovery target was chosen. Other - * variables store the actual target value (for example a string or a xid). - * The assign functions of the parameters check whether a competing parameter - * was already set. But we want to allow setting the same parameter multiple - * times. We also want to allow unsetting a parameter and setting a different - * one, so we unset recoveryTarget when the parameter is set to an empty - * string. - * - * XXX this code is broken by design. Throwing an error from a GUC assign - * hook breaks fundamental assumptions of guc.c. So long as all the variables - * for which this can happen are PGC_POSTMASTER, the consequences are limited, - * since we'd just abort postmaster startup anyway. Nonetheless it's likely - * that we have odd behaviors such as unexpected GUC ordering dependencies. + * Return the recovery target derived from the recovery_target* settings, + * raising an error if more than one of them is set. */ - -pg_noreturn static void -error_multiple_recovery_targets(void) +static RecoveryTargetType +DetermineRecoveryTargetType(void) { - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("multiple recovery targets specified"), - errdetail("At most one of \"recovery_target\", \"recovery_target_lsn\", \"recovery_target_name\", \"recovery_target_time\", \"recovery_target_xid\" may be set."))); + int ntargets = 0; + RecoveryTargetType target = RECOVERY_TARGET_UNSET; + const char *val; + StringInfoData buf; + + initStringInfo(&buf); + +#define ADD_TARGET_IF_SET(gucname, kind) \ + do { \ + val = GetConfigOption(gucname, false, false); \ + if (val[0] != '\0') \ + { \ + ntargets++; \ + target = (kind); \ + if (buf.len == 0) \ + appendStringInfo(&buf, _("\"%s\""), gucname); \ + else \ + appendStringInfo(&buf, _(", \"%s\""), gucname); \ + } \ + } while (0) + + ADD_TARGET_IF_SET("recovery_target", RECOVERY_TARGET_IMMEDIATE); + ADD_TARGET_IF_SET("recovery_target_lsn", RECOVERY_TARGET_LSN); + ADD_TARGET_IF_SET("recovery_target_name", RECOVERY_TARGET_NAME); + ADD_TARGET_IF_SET("recovery_target_time", RECOVERY_TARGET_TIME); + ADD_TARGET_IF_SET("recovery_target_xid", RECOVERY_TARGET_XID); +#undef ADD_TARGET_IF_SET + + if (ntargets > 1) + ereport(FATAL, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot specify more than one recovery target"), + errdetail("Parameters set are: %s.", + buf.data)); + + pfree(buf.data); + + return target; } /* @@ -4809,22 +4834,6 @@ check_recovery_target(char **newval, void **extra, GucSource source) return true; } -/* - * GUC assign_hook for recovery_target - */ -void -assign_recovery_target(const char *newval, void *extra) -{ - if (recoveryTarget != RECOVERY_TARGET_UNSET && - recoveryTarget != RECOVERY_TARGET_IMMEDIATE) - error_multiple_recovery_targets(); - - if (newval && strcmp(newval, "") != 0) - recoveryTarget = RECOVERY_TARGET_IMMEDIATE; - else - recoveryTarget = RECOVERY_TARGET_UNSET; -} - /* * GUC check_hook for recovery_target_lsn */ @@ -4856,17 +4865,8 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source) void assign_recovery_target_lsn(const char *newval, void *extra) { - if (recoveryTarget != RECOVERY_TARGET_UNSET && - recoveryTarget != RECOVERY_TARGET_LSN) - error_multiple_recovery_targets(); - if (newval && strcmp(newval, "") != 0) - { - recoveryTarget = RECOVERY_TARGET_LSN; recoveryTargetLSN = *((XLogRecPtr *) extra); - } - else - recoveryTarget = RECOVERY_TARGET_UNSET; } /* @@ -4885,25 +4885,6 @@ check_recovery_target_name(char **newval, void **extra, GucSource source) return true; } -/* - * GUC assign_hook for recovery_target_name - */ -void -assign_recovery_target_name(const char *newval, void *extra) -{ - if (recoveryTarget != RECOVERY_TARGET_UNSET && - recoveryTarget != RECOVERY_TARGET_NAME) - error_multiple_recovery_targets(); - - if (newval && strcmp(newval, "") != 0) - { - recoveryTarget = RECOVERY_TARGET_NAME; - recoveryTargetName = newval; - } - else - recoveryTarget = RECOVERY_TARGET_UNSET; -} - /* * GUC check_hook for recovery_target_time * @@ -4965,22 +4946,6 @@ check_recovery_target_time(char **newval, void **extra, GucSource source) return true; } -/* - * GUC assign_hook for recovery_target_time - */ -void -assign_recovery_target_time(const char *newval, void *extra) -{ - if (recoveryTarget != RECOVERY_TARGET_UNSET && - recoveryTarget != RECOVERY_TARGET_TIME) - error_multiple_recovery_targets(); - - if (newval && strcmp(newval, "") != 0) - recoveryTarget = RECOVERY_TARGET_TIME; - else - recoveryTarget = RECOVERY_TARGET_UNSET; -} - /* * GUC check_hook for recovery_target_timeline */ @@ -5099,15 +5064,6 @@ check_recovery_target_xid(char **newval, void **extra, GucSource source) void assign_recovery_target_xid(const char *newval, void *extra) { - if (recoveryTarget != RECOVERY_TARGET_UNSET && - recoveryTarget != RECOVERY_TARGET_XID) - error_multiple_recovery_targets(); - if (newval && strcmp(newval, "") != 0) - { - recoveryTarget = RECOVERY_TARGET_XID; recoveryTargetXid = *((TransactionId *) extra); - } - else - recoveryTarget = RECOVERY_TARGET_UNSET; } diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index d421cdbde76..adb72361ce0 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2472,7 +2472,6 @@ variable => 'recovery_target_string', boot_val => '""', check_hook => 'check_recovery_target', - assign_hook => 'assign_recovery_target', }, { name => 'recovery_target_action', type => 'enum', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET', @@ -2498,10 +2497,9 @@ { name => 'recovery_target_name', type => 'string', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET', short_desc => 'Sets the named restore point up to which recovery will proceed.', - variable => 'recovery_target_name_string', + variable => 'recoveryTargetName', boot_val => '""', check_hook => 'check_recovery_target_name', - assign_hook => 'assign_recovery_target_name', }, { name => 'recovery_target_time', type => 'string', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET', @@ -2509,7 +2507,6 @@ variable => 'recovery_target_time_string', boot_val => '""', check_hook => 'check_recovery_target_time', - assign_hook => 'assign_recovery_target_time', }, { name => 'recovery_target_timeline', type => 'string', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET', diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 90aa374b3ec..1ec460b6a82 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -667,7 +667,6 @@ static bool exec_backend_enabled = EXEC_BACKEND_ENABLED; static char *recovery_target_timeline_string; static char *recovery_target_string; static char *recovery_target_xid_string; -static char *recovery_target_name_string; static char *recovery_target_lsn_string; /* should be static, but commands/variable.c needs to get at this */ diff --git a/src/include/access/xlogrecovery.h b/src/include/access/xlogrecovery.h index ba7750dca0b..9ffd44fcbae 100644 --- a/src/include/access/xlogrecovery.h +++ b/src/include/access/xlogrecovery.h @@ -139,7 +139,7 @@ extern PGDLLIMPORT char *archiveCleanupCommand; extern PGDLLIMPORT TransactionId recoveryTargetXid; extern PGDLLIMPORT char *recovery_target_time_string; extern PGDLLIMPORT TimestampTz recoveryTargetTime; -extern PGDLLIMPORT const char *recoveryTargetName; +extern PGDLLIMPORT char *recoveryTargetName; extern PGDLLIMPORT XLogRecPtr recoveryTargetLSN; extern PGDLLIMPORT RecoveryTargetType recoveryTarget; extern PGDLLIMPORT bool wal_receiver_create_temp_slot; diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 307f4fbaefe..6a76f8d5ed6 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -103,16 +103,13 @@ extern bool check_recovery_prefetch(int *new_value, void **extra, extern void assign_recovery_prefetch(int new_value, void *extra); extern bool check_recovery_target(char **newval, void **extra, GucSource source); -extern void assign_recovery_target(const char *newval, void *extra); extern bool check_recovery_target_lsn(char **newval, void **extra, GucSource source); extern void assign_recovery_target_lsn(const char *newval, void *extra); extern bool check_recovery_target_name(char **newval, void **extra, GucSource source); -extern void assign_recovery_target_name(const char *newval, void *extra); extern bool check_recovery_target_time(char **newval, void **extra, GucSource source); -extern void assign_recovery_target_time(const char *newval, void *extra); extern bool check_recovery_target_timeline(char **newval, void **extra, GucSource source); extern void assign_recovery_target_timeline(const char *newval, void *extra); diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl index 047eb13293a..0469afb5bde 100644 --- a/src/test/recovery/t/003_recovery_targets.pl +++ b/src/test/recovery/t/003_recovery_targets.pl @@ -12,6 +12,10 @@ use Time::HiRes qw(usleep); # Create and test a standby from given backup, with a certain recovery target. # Choose $until_lsn later than the transaction commit that causes the row # count to reach $num_rows, yet not later than the recovery target. +# If options is given, pass it through Cluster->start(options => ...). This is +# used to exercise scenarios that require the postmaster command line to receive +# multiple "-c name=value" instances of the same GUC, which postgresql.conf +# cannot express because ProcessConfigFile collapses duplicate keys. sub test_recovery_standby { local $Test::Builder::Level = $Test::Builder::Level + 1; @@ -22,6 +26,7 @@ sub test_recovery_standby my $recovery_params = shift; my $num_rows = shift; my $until_lsn = shift; + my %params = @_; my $node_standby = PostgreSQL::Test::Cluster->new($node_name); $node_standby->init_from_backup($node_primary, 'my_backup', @@ -32,7 +37,8 @@ sub test_recovery_standby $node_standby->append_conf('postgresql.conf', qq($param_item)); } - $node_standby->start; + $node_standby->start( + defined $params{options} ? (options => $params{options}) : ()); # Wait until standby has replayed enough data my $caughtup_query = @@ -108,6 +114,9 @@ $node_primary->safe_psql('postgres', # Force archiving of WAL file $node_primary->safe_psql('postgres', "SELECT pg_switch_wal()"); +my $lsn6 = + $node_primary->safe_psql('postgres', "SELECT pg_current_wal_lsn()"); + # Test recovery targets my @recovery_params = ("recovery_target = 'immediate'"); test_recovery_standby('immediate target', @@ -125,11 +134,19 @@ test_recovery_standby('name', 'standby_4', $node_primary, \@recovery_params, test_recovery_standby('LSN', 'standby_5', $node_primary, \@recovery_params, "5000", $lsn5); +# Regression: empty-string for one recovery_target_* GUC must not clobber +# another non-empty target. Setting recovery_target_xid + recovery_target_time +# = '' must recover to the xid, not run as no-target recovery. +@recovery_params = + ("recovery_target_xid = '$recovery_txid'", "recovery_target_time = ''"); +test_recovery_standby('xid with empty time GUC', + 'standby_xid_empty_time', $node_primary, \@recovery_params, + "2000", $lsn2); + # Multiple targets # -# Multiple conflicting settings are not allowed, but setting the same -# parameter multiple times or unsetting a parameter and setting a -# different one is allowed. +# Multiple conflicting non-empty settings are rejected, but setting the same +# parameter twice or clearing one with an empty string is allowed. @recovery_params = ( "recovery_target_name = '$recovery_name'", @@ -138,31 +155,9 @@ test_recovery_standby('LSN', 'standby_5', $node_primary, \@recovery_params, test_recovery_standby('multiple overriding settings', 'standby_6', $node_primary, \@recovery_params, "3000", $lsn3); -my $node_standby = PostgreSQL::Test::Cluster->new('standby_7'); -$node_standby->init_from_backup($node_primary, 'my_backup', - has_restoring => 1); -$node_standby->append_conf( - 'postgresql.conf', "recovery_target_name = '$recovery_name' -recovery_target_time = '$recovery_time'"); - -my $res = run_log( - [ - 'pg_ctl', - '--pgdata' => $node_standby->data_dir, - '--log' => $node_standby->logfile, - 'start', - ]); -ok(!$res, 'invalid recovery startup fails'); - -my $logfile = slurp_file($node_standby->logfile()); -like( - $logfile, - qr/multiple recovery targets specified/, - 'multiple conflicting settings'); - # Check behavior when recovery ends before target is reached -$node_standby = PostgreSQL::Test::Cluster->new('standby_8'); +my $node_standby = PostgreSQL::Test::Cluster->new('standby_8'); $node_standby->init_from_backup( $node_primary, 'my_backup', has_restoring => 1, @@ -184,12 +179,84 @@ foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default) last if !-f $node_standby->data_dir . '/postmaster.pid'; usleep(100_000); } -$logfile = slurp_file($node_standby->logfile()); +my $logfile = slurp_file($node_standby->logfile()); like( $logfile, qr/FATAL: .* recovery ended before configured recovery target was reached/, 'recovery end before target reached is a fatal error'); +# Conflicts are rejected at every startup, even without recovery.signal. +# init_from_backup without has_restoring creates no recovery.signal, so this +# cluster would otherwise start as a plain primary; the conflict must still be +# caught. +my $node_no_signal = PostgreSQL::Test::Cluster->new('multi_target_no_signal'); +$node_no_signal->init_from_backup($node_primary, 'my_backup'); +$node_no_signal->append_conf( + 'postgresql.conf', "recovery_target_name = '$recovery_name' +recovery_target_time = '$recovery_time'"); + +ok( !$node_no_signal->start(fail_ok => 1), + 'server fails to start with conflicting recovery targets and no recovery.signal' +); + +my $logfile_no_signal = slurp_file($node_no_signal->logfile()); +like( + $logfile_no_signal, + qr/cannot specify more than one recovery target/, + 'expected error message logged without recovery.signal'); +like( + $logfile_no_signal, + qr/Parameters set are: "recovery_target_name", "recovery_target_time"/, + 'errdetail lists the set parameters in order without recovery.signal'); +unlike( + $logfile_no_signal, + qr/Parameters set are:[^\n]*=/, + 'errdetail does not echo parameter values without recovery.signal'); + +my $node_immediate_conflict = + PostgreSQL::Test::Cluster->new('immediate_target_conflict'); +$node_immediate_conflict->init_from_backup($node_primary, 'my_backup'); +$node_immediate_conflict->append_conf( + 'postgresql.conf', + "recovery_target = 'immediate' +recovery_target_xid = '$recovery_txid'"); + +ok( !$node_immediate_conflict->start(fail_ok => 1), + 'server fails to start with recovery_target=immediate and a second target' +); +like( + slurp_file($node_immediate_conflict->logfile()), + qr/cannot specify more than one recovery target/, + 'recovery_target=immediate conflicting with another target is rejected'); + +# Same-GUC set-then-clear: setting a recovery_target_* GUC and then setting the +# same GUC to an empty string leaves no target, so recovery runs to the end of +# WAL. Duplicate keys collapse in postgresql.conf, so "pg_ctl --options" passes +# both assignments on the postmaster command line. +test_recovery_standby( + 'recovery_target_xid set then cleared', + 'standby_xid_set_clear', + $node_primary, + [], + "6000", + $lsn6, + options => "-c recovery_target_xid=$recovery_txid -c recovery_target_xid=" +); + +# Set recovery_target_xid, then set and clear recovery_target_name. Only the +# xid remains, so recovery must stop at it rather than running to the end of WAL +# (a competing target that is set then cleared must not strand the first one). +test_recovery_standby( + 'recovery target preserved when a competing one is set then cleared', + 'standby_clobber_clear', + $node_primary, + [], + "2000", + $lsn2, + options => + "-c recovery_target_xid=$recovery_txid -c recovery_target_name=$recovery_name -c recovery_target_name=" +); + # Invalid recovery_target_timeline tests my ($result, $stdout, $stderr) = $node_primary->psql('postgres', "ALTER SYSTEM SET recovery_target_timeline TO 'bogus'"); -- 2.55.0