From ef7c377ea08d74c4cd279f507e4ab80afe6c5322 Mon Sep 17 00:00:00 2001 From: JoongHyuk Shin Date: Sun, 28 Jun 2026 18:25:45 +0900 Subject: [PATCH v9] Don't call ereport(ERROR) from recovery target GUC assign hooks A GUC assign hook must not raise an error, but the recovery_target* assign hooks did so when a second target was set. Instead, derive recoveryTarget once in validateRecoveryParameters() from the settled recovery_target* values, rejecting there a configuration that sets more than one target. The assign hooks now only store each parameter's own value, and recovery_target_name, which needs no further processing, uses its GUC variable directly without an assign hook. --- src/backend/access/transam/xlogrecovery.c | 139 ++++++----------- 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 | 161 ++++++++++++++++---- 6 files changed, 183 insertions(+), 128 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c0ae4d3f63f..e2ba09594dd 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; @@ -341,6 +342,7 @@ static void ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, Time static void EnableStandbyMode(void); static void readRecoverySignalFile(void); static void validateRecoveryParameters(void); +static RecoveryTargetType DetermineRecoveryTargetType(void); static bool read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI, bool *backupEndRequired, bool *backupFromStandby); @@ -1067,6 +1069,8 @@ readRecoverySignalFile(void) static void validateRecoveryParameters(void) { + recoveryTarget = DetermineRecoveryTargetType(); + if (!ArchiveRecoveryRequested) return; @@ -4769,30 +4773,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 +4833,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 +4864,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 +4884,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 +4945,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 +5063,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 c9118e71988..191c69c5bc7 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2465,7 +2465,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', @@ -2491,10 +2490,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', @@ -2502,7 +2500,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..71281808983 100644 --- a/src/test/recovery/t/003_recovery_targets.pl +++ b/src/test/recovery/t/003_recovery_targets.pl @@ -51,6 +51,49 @@ sub test_recovery_standby return; } +# Start a standby with the given pg_ctl --options string and verify that +# the standby reaches the given LSN and row count. 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_with_options +{ + local $Test::Builder::Level = $Test::Builder::Level + 1; + + my $test_name = shift; + my $node_name = shift; + my $node_primary = shift; + my $options = shift; + my $num_rows = shift; + my $until_lsn = shift; + + my $node_standby = PostgreSQL::Test::Cluster->new($node_name); + $node_standby->init_from_backup($node_primary, 'my_backup', + has_restoring => 1); + + my $res = run_log( + [ + 'pg_ctl', + '--pgdata' => $node_standby->data_dir, + '--log' => $node_standby->logfile, + '--options' => $options, + 'start', + ]); + ok($res, "server starts for $test_name"); + + $node_standby->poll_query_until('postgres', + "SELECT '$until_lsn'::pg_lsn <= pg_last_wal_replay_lsn()") + or die "Timed out while waiting for standby to catch up"; + + my $count = $node_standby->safe_psql('postgres', + "SELECT count(*) FROM tab_int"); + is($count, qq($num_rows), "check standby content for $test_name"); + + $node_standby->teardown_node; + + return; +} + # Initialize primary node my $node_primary = PostgreSQL::Test::Cluster->new('primary'); $node_primary->init(has_archiving => 1, allows_streaming => 1); @@ -108,6 +151,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 +171,20 @@ 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 +193,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 +217,86 @@ 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'"); + +my $res_no_signal = run_log( + [ + 'pg_ctl', + '--pgdata' => $node_no_signal->data_dir, + '--log' => $node_no_signal->logfile, + 'start', + ]); +ok(!$res_no_signal, + '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'"); + +my $res_immediate = run_log( + [ + 'pg_ctl', + '--pgdata' => $node_immediate_conflict->data_dir, + '--log' => $node_immediate_conflict->logfile, + 'start', + ]); +ok(!$res_immediate, + '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_with_options( + 'recovery_target_xid set then cleared', + 'standby_xid_set_clear', $node_primary, + "-c recovery_target_xid=$recovery_txid -c recovery_target_xid=", + "6000", $lsn6); + +# 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_with_options( + 'recovery target preserved when a competing one is set then cleared', + 'standby_clobber_clear', $node_primary, + "-c recovery_target_xid=$recovery_txid -c recovery_target_name=$recovery_name -c recovery_target_name=", + "2000", $lsn2); + # Invalid recovery_target_timeline tests my ($result, $stdout, $stderr) = $node_primary->psql('postgres', "ALTER SYSTEM SET recovery_target_timeline TO 'bogus'"); -- 2.52.0