v7-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch

From 2c0daee6be5ffdb2ac48e4da687247480ebfb8dc Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <sjh910805@gmail.com>
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