From 2676f4ad037fec97be49dd4099b65bd8cf569119 Mon Sep 17 00:00:00 2001 From: JoongHyuk Shin Date: Wed, 29 Apr 2026 15:32:21 +0900 Subject: [PATCH v2] Don't call ereport(ERROR) from recovery target GUC assign hooks The five recovery target GUC assign hooks (assign_recovery_target, assign_recovery_target_lsn, assign_recovery_target_name, assign_recovery_target_time, assign_recovery_target_xid) all called error_multiple_recovery_targets(), which invoked ereport(ERROR). The GUC README explicitly states that assign hooks must never fail; raising an error from an assign hook leaves guc.c's internal state inconsistent before the abort. A comment in the code itself acknowledged this as "broken by design." Fix this by removing the conflict check from all five assign hooks and detecting multiple recovery targets in a new CheckRecoveryTargetConflicts() function, called from validateRecoveryParameters() before its early return for !ArchiveRecoveryRequested. The check therefore runs at every startup regardless of recovery mode, preserving the existing behavior of detecting misconfiguration at server start time (rather than only when recovery.signal is added). Also remove the unconditional `recoveryTarget = RECOVERY_TARGET_UNSET` fallback in each assign hook's empty-string branch. An empty string is the GUCs' default value and is now treated as a no-op rather than as an explicit "unset all targets" signal. This fixes a corner case where setting one GUC to a value and another to an empty string would silently clobber the first GUC's target, leaving recovery without any target. This corner case was reported by Fujii Masao during v1 review. Reported-by: Fujii Masao Discussion: https://postgr.es/m/CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com --- src/backend/access/transam/xlogrecovery.c | 118 +++++++++++--------- src/test/recovery/t/003_recovery_targets.pl | 48 +++++++- 2 files changed, 111 insertions(+), 55 deletions(-) diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index c236e2b7969..5cabd079d57 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -341,6 +341,7 @@ static void ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, Time static void EnableStandbyMode(void); static void readRecoverySignalFile(void); static void validateRecoveryParameters(void); +static void CheckRecoveryTargetConflicts(void); static bool read_backup_label(XLogRecPtr *checkPointLoc, TimeLineID *backupLabelTLI, bool *backupEndRequired, bool *backupFromStandby); @@ -1067,6 +1068,8 @@ readRecoverySignalFile(void) static void validateRecoveryParameters(void) { + CheckRecoveryTargetConflicts(); + if (!ArchiveRecoveryRequested) return; @@ -1144,6 +1147,62 @@ validateRecoveryParameters(void) } } +/* + * CheckRecoveryTargetConflicts + * + * Validate that at most one of the recovery_target_* GUCs is set to a + * non-empty value. This is called from validateRecoveryParameters() at every + * server startup, regardless of whether archive recovery is requested, so + * misconfiguration is detected at server start time rather than later when + * recovery.signal is added. + * + * The check is a separate function rather than inlined into + * validateRecoveryParameters() because it intentionally runs even when no + * recovery is requested, while the rest of validateRecoveryParameters() is + * recovery-mode-only. Keeping it as a named function makes that separation + * explicit. + * + * The check used to live in the assign hooks of the recovery_target_* GUCs + * (calling ereport(ERROR) on conflict), which violated guc.c's contract that + * assign hooks must never fail. Moving the check here keeps the assign hooks + * contract-compliant. + * + * If a future patch adds a sixth recovery_target_* GUC, both this list and + * the errdetail below must be updated. + */ +static void +CheckRecoveryTargetConflicts(void) +{ + int ntargets = 0; + const char *val; + + val = GetConfigOption("recovery_target", false, false); + if (val != NULL && val[0] != '\0') + ntargets++; + val = GetConfigOption("recovery_target_lsn", false, false); + if (val != NULL && val[0] != '\0') + ntargets++; + val = GetConfigOption("recovery_target_name", false, false); + if (val != NULL && val[0] != '\0') + ntargets++; + val = GetConfigOption("recovery_target_time", false, false); + if (val != NULL && val[0] != '\0') + ntargets++; + val = GetConfigOption("recovery_target_xid", false, false); + if (val != NULL && val[0] != '\0') + ntargets++; + + if (ntargets > 1) + ereport(FATAL, + (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\" can be set."))); +} + /* * read_backup_label: check to see if a backup_label file is present * @@ -4764,32 +4823,17 @@ 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 + * Recovery target settings: At most one of the several recovery_target* + * settings may be set to a non-empty value. 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. + * An empty string for any of these GUCs is treated as "not set", equivalent + * to the GUC's default; setting a GUC to an empty string is a no-op and does + * not clear other already-set targets. Conflicts between multiple non-empty + * settings are detected in CheckRecoveryTargetConflicts(), called from + * validateRecoveryParameters() at every startup. */ -pg_noreturn static void -error_multiple_recovery_targets(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."))); -} - /* * GUC check_hook for recovery_target */ @@ -4810,14 +4854,8 @@ check_recovery_target(char **newval, void **extra, GucSource source) 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; } /* @@ -4851,17 +4889,11 @@ 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; } /* @@ -4886,17 +4918,11 @@ 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; } /* @@ -4966,14 +4992,8 @@ check_recovery_target_time(char **newval, void **extra, GucSource source) 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; } /* @@ -5094,15 +5114,9 @@ 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/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl index 047eb13293a..ef22a80252b 100644 --- a/src/test/recovery/t/003_recovery_targets.pl +++ b/src/test/recovery/t/003_recovery_targets.pl @@ -125,11 +125,24 @@ 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 not allowed. Setting the same +# parameter multiple times is allowed (the last value wins, per ProcessConfigFile +# duplicate handling). An empty string for a recovery_target_* GUC is treated +# as the GUC's default and is a no-op; it does not clear any other already-set +# target. Conflict detection runs in CheckRecoveryTargetConflicts() at every +# server start, regardless of whether recovery is requested. @recovery_params = ( "recovery_target_name = '$recovery_name'", @@ -190,6 +203,35 @@ like( qr/FATAL: .* recovery ended before configured recovery target was reached/, 'recovery end before target reached is a fatal error'); +# Conflicting recovery targets are rejected at every startup, regardless of +# whether recovery.signal is present. Use a throwaway cluster initialized +# from the existing backup so we can leave conflicting GUCs in its +# postgresql.conf without polluting the primary used by later tests. +# init_from_backup is called without has_restoring, so no recovery.signal +# is created and the cluster would normally start as a plain primary; the +# conflicting recovery_target_* GUCs must be rejected anyway. +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'); + # Invalid recovery_target_timeline tests my ($result, $stdout, $stderr) = $node_primary->psql('postgres', "ALTER SYSTEM SET recovery_target_timeline TO 'bogus'"); -- 2.52.0