public inbox for [email protected]
help / color / mirror / Atom feedFrom: Scott Ray <[email protected]>
To: JoongHyuk Shin <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
Date: Sat, 06 Jun 2026 20:10:10 +0000
Message-ID: <0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io> (raw)
In-Reply-To: <CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com>
References: <CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com>
<CAHGQGwFNjrLfUSyT4McR4wH_2yFV3n7+63eyvDjzxQchyyQrPA@mail.gmail.com>
<[email protected]>
<CACSdjfPCPjP9tuzxgarcbkjahu-n+o+LJbUQ22aPNkOsOvrEZg@mail.gmail.com>
<CAHGQGwF24XMh=35m+FdqLUw4QgK8HmAGwPZyBDKtsw7MWYp_hw@mail.gmail.com>
<CACSdjfNY=aePEYGYV_5tamz-iTLQLitBuHc8youfsTzfDR1+_g@mail.gmail.com>
<CACSdjfMohHudUh7Fq=pD=Zk+=J=4E65c6VGgScG3CAXvj_Sorg@mail.gmail.com>
<M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io>
<CACSdjfOxnez=Cv8CzvJcupnaSjaQFnXqk5t=C1ES0BXp52faFg@mail.gmail.com>
> * Function structure: the recovery_target_* set has been
> historically stable, so array + loop abstraction adds limited
> value; function size grows ~34% (32 -> 43 lines) for one line of
> savings on a hypothetical sixth GUC, while the closest precedent
> (archive_command / archive_library in pgarch.c) is a hard-coded literal.
>
> * errhint vs errdetail: errhint("At most one of %s can be set.")
> reads more like a constraint than an action hint. The closest
> precedent, archive_command / archive_library in pgarch.c
> (ProcessPgArchInterrupts() / LoadArchiveLibrary()), keeps the
> enumeration in errdetail and omits errhint entirely.
>
> * TAP regex: the added like() uses [^"]+ for the values, which
> passes regardless of the actual value. Using quotemeta on the
> expected values would verify the actual content, and anchoring
> would also avoid accidentally matching the same tokens inside
> errhint.
Thanks for taking a look. I attached a v2 that applies your suggestions
and uses "set to" instead of "=" to match convention. What do you think?
Sample output:
FATAL: multiple recovery targets specified
DETAIL: At most one of "recovery_target", "recovery_target_lsn",
"recovery_target_name", "recovery_target_time",
"recovery_target_xid" can be set. Currently set:
"recovery_target_time" set to "2026-01-01 00:00:00",
"recovery_target_xid" set to "700".
--
Scott Ray
Attachments:
[application/octet-stream] v2-0001-Report-set-parameters-on-recovery_target-conflict.patch (6.0K, 2-v2-0001-Report-set-parameters-on-recovery_target-conflict.patch)
download | inline diff:
From 9b96e0f327d7ca2f644b259511776ef7286ec72f Mon Sep 17 00:00:00 2001
From: Scott Ray <[email protected]>
Date: Sun, 31 May 2026 13:12:29 -0700
Subject: [PATCH v2] 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 appends the names and values of the GUCs that are
actually non-empty as a trailing sentence in the existing errdetail.
The "at most one of [list]" enumeration stays in errdetail and no
errhint is emitted, matching the archive_command / archive_library
precedent in pgarch.c.
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, using quotemeta on the expected values.
Applies atop v4.
Discussion: https://postgr.es/m/CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com
---
src/backend/access/transam/xlogrecovery.c | 35 ++++++++++++++++--
src/test/recovery/t/003_recovery_targets.pl | 40 +++++++++++++++++++++
2 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 1253bed1058..6af36960ddc 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1167,31 +1167,58 @@ 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 another
+ * GetConfigOption block below to include it in the "Currently set:"
+ * suffix, and extend the fixed enumeration in the errdetail.
*/
static void
CheckRecoveryTargetConflicts(void)
{
+ StringInfoData set_targets;
int ntargets = 0;
const char *val;
+ initStringInfo(&set_targets);
+
/* missing_ok=false guarantees val is non-NULL. */
val = GetConfigOption("recovery_target", false, false);
if (val[0] != '\0')
+ {
+ appendStringInfo(&set_targets, "\"recovery_target\" set to \"%s\"", val);
ntargets++;
+ }
val = GetConfigOption("recovery_target_lsn", false, false);
if (val[0] != '\0')
+ {
+ if (ntargets > 0)
+ appendStringInfoString(&set_targets, ", ");
+ appendStringInfo(&set_targets, "\"recovery_target_lsn\" set to \"%s\"", val);
ntargets++;
+ }
val = GetConfigOption("recovery_target_name", false, false);
if (val[0] != '\0')
+ {
+ if (ntargets > 0)
+ appendStringInfoString(&set_targets, ", ");
+ appendStringInfo(&set_targets, "\"recovery_target_name\" set to \"%s\"", val);
ntargets++;
+ }
val = GetConfigOption("recovery_target_time", false, false);
if (val[0] != '\0')
+ {
+ if (ntargets > 0)
+ appendStringInfoString(&set_targets, ", ");
+ appendStringInfo(&set_targets, "\"recovery_target_time\" set to \"%s\"", val);
ntargets++;
+ }
val = GetConfigOption("recovery_target_xid", false, false);
if (val[0] != '\0')
+ {
+ if (ntargets > 0)
+ appendStringInfoString(&set_targets, ", ");
+ appendStringInfo(&set_targets, "\"recovery_target_xid\" set to \"%s\"", val);
ntargets++;
+ }
if (ntargets > 1)
ereport(FATAL,
@@ -1201,7 +1228,9 @@ CheckRecoveryTargetConflicts(void)
"\"recovery_target_lsn\", "
"\"recovery_target_name\", "
"\"recovery_target_time\", "
- "\"recovery_target_xid\" can be set.")));
+ "\"recovery_target_xid\" can be set. "
+ "Currently set: %s.",
+ set_targets.data)));
}
/*
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 5979663b0ab..f00a82d4e9e 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 the GetConfigOption sequence in
+# CheckRecoveryTargetConflicts: recovery_target, recovery_target_lsn,
+# recovery_target_name, recovery_target_time, recovery_target_xid.
+like(
+ $logfile_no_signal,
+ qr/Currently set: "recovery_target_name" set to "\Q$recovery_name\E", "recovery_target_time" set to "\Q$recovery_time\E"/,
+ '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)
[application/pgp-signature] signature.asc (343B, 3-signature.asc)
download
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
In-Reply-To: <0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox