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

