v1-0001-Report-set-parameters-on-recovery_target-conflict.patch

From c61284c824429778ad8f123e22862931d41a2a6d Mon Sep 17 00:00:00 2001
From: Scott Ray <scott@scottray.io>
Date: Sun, 31 May 2026 13:12:29 -0700
Subject: [PATCH v1] Report set parameters on recovery_target conflict; expand
 tests

v4 of "Don't call ereport(ERROR) from recovery target GUC assign
hooks" produces a FATAL with an errdetail that lists all five
recovery_target_* GUCs regardless of which the operator actually
set, and exercises only recovery_target_xid in the cleared-then-set
direction.

This patch makes CheckRecoveryTargetConflicts() report the names
and values of the GUCs that are actually non-empty in errdetail,
moving the "at most one of [list]" enumeration to errhint.  The
five hand-written GetConfigOption() calls collapse into a loop over
a static target_names[] array, so adding a sixth recovery_target_*
GUC requires only updating the array; both error messages are
derived from it.

The TAP test gains four cleared-then-set cases covering time, name,
lsn, and the bare recovery_target, mirroring the existing xid case.
A new like() assertion verifies that the errdetail names which GUCs
are set and their values.

Applies atop v4.

Discussion: https://postgr.es/m/CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com
---
 src/backend/access/transam/xlogrecovery.c   | 61 ++++++++++++---------
 src/test/recovery/t/003_recovery_targets.pl | 40 ++++++++++++++
 2 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 1253bed1058..e48e21631b2 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1167,41 +1167,52 @@ validateRecoveryParameters(void)
  * 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.
+ * If a future patch adds a sixth recovery_target_* GUC, add its name to
+ * target_names below; both error messages are derived from that array.
  */
 static void
 CheckRecoveryTargetConflicts(void)
 {
+	static const char *const target_names[] = {
+		"recovery_target",
+		"recovery_target_lsn",
+		"recovery_target_name",
+		"recovery_target_time",
+		"recovery_target_xid",
+	};
+	StringInfoData set_targets;
+	StringInfoData all_targets;
 	int			ntargets = 0;
-	const char *val;
-
-	/* missing_ok=false guarantees val is non-NULL. */
-	val = GetConfigOption("recovery_target", false, false);
-	if (val[0] != '\0')
-		ntargets++;
-	val = GetConfigOption("recovery_target_lsn", false, false);
-	if (val[0] != '\0')
-		ntargets++;
-	val = GetConfigOption("recovery_target_name", false, false);
-	if (val[0] != '\0')
-		ntargets++;
-	val = GetConfigOption("recovery_target_time", false, false);
-	if (val[0] != '\0')
-		ntargets++;
-	val = GetConfigOption("recovery_target_xid", false, false);
-	if (val[0] != '\0')
-		ntargets++;
+
+	initStringInfo(&set_targets);
+	initStringInfo(&all_targets);
+
+	for (int i = 0; i < lengthof(target_names); i++)
+	{
+		/* missing_ok=false guarantees val is non-NULL. */
+		const char *val = GetConfigOption(target_names[i], false, false);
+
+		if (i > 0)
+			appendStringInfoString(&all_targets, ", ");
+		appendStringInfo(&all_targets, "\"%s\"", target_names[i]);
+
+		if (val[0] != '\0')
+		{
+			if (ntargets > 0)
+				appendStringInfoString(&set_targets, ", ");
+			appendStringInfo(&set_targets, "\"%s\" = \"%s\"",
+							 target_names[i], val);
+			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.")));
+				 errdetail("The following recovery target parameters are set: %s.",
+						   set_targets.data),
+				 errhint("At most one of %s can be set.", all_targets.data)));
 }
 
 /*
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 5979663b0ab..3e68c01968b 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -282,6 +282,14 @@ like(
 	qr/multiple recovery targets specified/,
 	'expected error message logged without recovery.signal');
 
+# Ordering in the errdetail follows target_names[] in CheckRecoveryTargetConflicts:
+# recovery_target, recovery_target_lsn, recovery_target_name,
+# recovery_target_time, recovery_target_xid.
+like(
+	$logfile_no_signal,
+	qr/are set: "recovery_target_name" = "[^"]+", "recovery_target_time" = "[^"]+"/,
+	'errdetail names which recovery_target_* GUCs are set and their values');
+
 # Same-GUC last-wins (one source of truth for the GUC's value): assigning a
 # recovery_target_* GUC and then assigning the same GUC to an empty string
 # leaves no target set and recovery proceeds to the end of WAL.  This is the
@@ -358,6 +366,38 @@ is($count_xid_clear_set, "2000",
 	'recovery_target_xid honored when cleared then set');
 $node_xid_clear_set->teardown_node;
 
+test_recovery_standby_with_options(
+	'recovery_target_time cleared then set',
+	'standby_time_clear_set',
+	$node_primary,
+	"-c recovery_target_time= -c recovery_target_time=$recovery_time_t",
+	"3000",
+	$lsn3);
+
+test_recovery_standby_with_options(
+	'recovery_target_name cleared then set',
+	'standby_name_clear_set',
+	$node_primary,
+	"-c recovery_target_name= -c recovery_target_name=$recovery_name",
+	"4000",
+	$lsn4);
+
+test_recovery_standby_with_options(
+	'recovery_target_lsn cleared then set',
+	'standby_lsn_clear_set',
+	$node_primary,
+	"-c recovery_target_lsn= -c recovery_target_lsn=$recovery_lsn",
+	"5000",
+	$lsn5);
+
+test_recovery_standby_with_options(
+	'recovery_target cleared then set',
+	'standby_immediate_clear_set',
+	$node_primary,
+	"-c recovery_target= -c recovery_target=immediate",
+	"1000",
+	$lsn1);
+
 # Invalid recovery_target_timeline tests
 my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
 	"ALTER SYSTEM SET recovery_target_timeline TO 'bogus'");
-- 
2.50.1 (Apple Git-155)