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

From a4be55d99de64e47c368f7512d47164803467a03 Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <sjh910805@gmail.com>
Date: Mon, 13 Apr 2026 16:52:26 +0900
Subject: [PATCH] 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 validateRecoveryParameters()
instead.  That function already runs after all GUCs have been loaded
(called from InitWalRecovery() in the startup process), so it can read
each GUC's current value via GetConfigOption() and count how many are
non-empty.  If more than one is set, it reports FATAL, which is
consistent with the other validation errors in that function.

This changes the timing of the error: it now fires in the startup
process rather than in the postmaster's ProcessConfigFile.  The outcome
is the same (server does not start), and the GUC infrastructure is left
in a consistent state.

A secondary behavioral change: conflicting recovery target settings are
now silently ignored when ArchiveRecoveryRequested is false (i.e., when
recovery is not actually requested).  The previous code would reject
them even in normal startup, which was unnecessary since those GUCs have
no effect when recovery is not requested.  A TAP test is added for this
new behavior.
---
 src/backend/access/transam/xlogrecovery.c   | 87 +++++++++++----------
 src/test/recovery/t/003_recovery_targets.pl | 22 ++++++
 2 files changed, 66 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c236e2b7969..65eeb1214df 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1070,6 +1070,43 @@ validateRecoveryParameters(void)
 	if (!ArchiveRecoveryRequested)
 		return;
 
+	/*
+	 * Check for conflicting recovery targets.  We do this here rather than in
+	 * the GUC assign hooks because throwing an error from an assign hook
+	 * violates guc.c's contract.  By the time we reach this function, all
+	 * GUCs have been loaded, so we can safely read their current values.
+	 */
+	{
+		int			ntargets = 0;
+		const char *val;
+
+		val = GetConfigOption("recovery_target", true, false);
+		if (val != NULL && val[0] != '\0')
+			ntargets++;
+		val = GetConfigOption("recovery_target_lsn", true, false);
+		if (val != NULL && val[0] != '\0')
+			ntargets++;
+		val = GetConfigOption("recovery_target_name", true, false);
+		if (val != NULL && val[0] != '\0')
+			ntargets++;
+		val = GetConfigOption("recovery_target_time", true, false);
+		if (val != NULL && val[0] != '\0')
+			ntargets++;
+		val = GetConfigOption("recovery_target_xid", true, 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\" may be set.")));
+	}
+
 	/*
 	 * Check for compulsory parameters
 	 */
@@ -4765,31 +4802,15 @@ 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.
+ * may be set.  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).  We want to allow setting the same
+ * parameter multiple times, and we want to allow unsetting a parameter and
+ * setting a different one, so we unset recoveryTarget when the parameter is
+ * set to an empty string.  Conflicts between multiple non-empty settings are
+ * detected in validateRecoveryParameters().
  */
 
-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,10 +4831,6 @@ 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
@@ -4851,10 +4868,6 @@ 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;
@@ -4886,10 +4899,6 @@ 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;
@@ -4966,10 +4975,6 @@ 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
@@ -5094,10 +5099,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;
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 047eb13293a..4014c28bf95 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -190,6 +190,28 @@ like(
 	qr/FATAL: .* recovery ended before configured recovery target was reached/,
 	'recovery end before target reached is a fatal error');
 
+# Conflicting recovery targets without recovery mode
+#
+# When recovery is not requested (no recovery.signal), conflicting recovery
+# target settings should be silently accepted, since those GUCs have no
+# effect outside of recovery.
+$node_primary->stop;
+$node_primary->append_conf(
+	'postgresql.conf', "recovery_target_name = '$recovery_name'
+recovery_target_time = '$recovery_time'");
+$node_primary->start;
+my $primary_pid = $node_primary->safe_psql('postgres',
+	'SELECT pg_backend_pid()');
+ok(defined $primary_pid && $primary_pid ne '',
+	'server starts despite conflicting recovery targets outside recovery');
+
+# Clean up the conflicting settings before continuing
+$node_primary->safe_psql('postgres',
+	"ALTER SYSTEM RESET recovery_target_name");
+$node_primary->safe_psql('postgres',
+	"ALTER SYSTEM RESET recovery_target_time");
+$node_primary->restart;
+
 # Invalid recovery_target_timeline tests
 my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
 	"ALTER SYSTEM SET recovery_target_timeline TO 'bogus'");
-- 
2.52.0