From 2c0daee6be5ffdb2ac48e4da687247480ebfb8dc Mon Sep 17 00:00:00 2001 From: JoongHyuk Shin Date: Sun, 28 Jun 2026 18:25:45 +0900 Subject: [PATCH] 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. Make the assign hooks store only their own value, and derive recoveryTarget once in validateRecoveryParameters() from the settled recovery_target* values, rejecting there a configuration that sets more than one target. --- src/backend/access/transam/xlogrecovery.c | 142 ++++++++----------- src/backend/utils/misc/guc_parameters.dat | 2 - src/include/utils/guc_hooks.h | 2 - src/test/recovery/t/003_recovery_targets.pl | 149 ++++++++++++++++---- 4 files changed, 183 insertions(+), 112 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c0ae4d3f63f..5c8d019c7bb 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" @@ -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,14 @@ readRecoverySignalFile(void) static void validateRecoveryParameters(void) { + /* + * Derive recoveryTarget from the final recovery_target* settings, + * rejecting a configuration with more than one of them. This runs before + * the early return below so that conflicts are rejected at every startup, + * as the assign hooks used to do. + */ + recoveryTarget = DetermineRecoveryTargetType(); + if (!ArchiveRecoveryRequested) return; @@ -4769,30 +4779,59 @@ 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. + * Recovery target settings: at most one of the recovery_target* settings may + * be set. The assign hooks just store each parameter's own value; the chosen + * target and any conflict are derived here instead, from the final settings, + * because an assign hook must not raise an error and cannot see sibling GUCs. + * validateRecoveryParameters() calls this once after all GUC processing. */ - -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); + + /* + * These are all PGC_STRING, so GetConfigOption() returns "" (not NULL) + * when unset. The separators and quotes are wrapped in _() so + * translators can adapt the list punctuation. + */ +#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("multiple recovery targets specified"), + errdetail("Only one recovery target can be set. Parameters set: %s.", + buf.data), + errhint("See pg_settings for the parameter values and where each is set.")); + + pfree(buf.data); + + return target; } /* @@ -4809,22 +4848,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 +4879,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; } /* @@ -4891,17 +4905,8 @@ check_recovery_target_name(char **newval, void **extra, GucSource source) 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; } /* @@ -4965,22 +4970,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 +5088,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 3c1e6b31bf8..7bc967c629f 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2455,7 +2455,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', @@ -2492,7 +2491,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/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 307f4fbaefe..1aec17c67bd 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -103,7 +103,6 @@ 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); @@ -112,7 +111,6 @@ extern bool check_recovery_target_name(char **newval, void **extra, 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..f4d612e4263 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,12 @@ $node_primary->safe_psql('postgres', # Force archiving of WAL file $node_primary->safe_psql('postgres', "SELECT pg_switch_wal()"); +# LSN after the final 6000-row insert and WAL switch. The set-then-clear case +# below has no recovery target and replays all WAL, so it polls on this instead +# of $lsn5, which would race the 5001-6000 rows. +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 +174,22 @@ 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. Setting the same +# parameter twice is allowed (last value wins), and an empty string is a no-op +# that does not clear another GUC's target. Conflicts are detected at every +# server start by DetermineRecoveryTargetType(). @recovery_params = ( "recovery_target_name = '$recovery_name'", @@ -138,31 +198,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 +222,69 @@ 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/multiple recovery targets specified/, + 'expected error message logged without recovery.signal'); +like( + $logfile_no_signal, + qr/Only one recovery target can be set\. Parameters set: "recovery_target_name", "recovery_target_time"/, + 'errdetail lists the set parameters in order without recovery.signal'); +unlike( + $logfile_no_signal, + qr/Parameters set:[^\n]*=/, + 'errdetail does not echo parameter values without recovery.signal'); +like( + $logfile_no_signal, + qr/HINT:.*pg_settings/, + 'errhint points to pg_settings without recovery.signal'); + +# 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