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: Sun, 31 May 2026 21:11:07 +0000
Message-ID: <M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io> (raw)
In-Reply-To: <CACSdjfMohHudUh7Fq=pD=Zk+=J=4E65c6VGgScG3CAXvj_Sorg@mail.gmail.com>
References: <CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com>
<CAHGQGwExsY9XvxN=u4ip7pA+_gO-ms8EwgcRZF3Nj84Vi8yeBQ@mail.gmail.com>
<[email protected]>
<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>
Thanks for the patch. I've attached v1-0001 (atop v4) addressing the
UX and test-coverage items below. Happy to rework or fold in however
you prefer.
1. There's a configuration trap in master and in this branch that
could be prevented using something very similar to
CheckRecoveryTargetConflicts to check pending GUCs:
psql -c "ALTER SYSTEM SET recovery_target_xid TO '700'"
psql -c "ALTER SYSTEM SET recovery_target_time TO '2026-01-01 00:00:00'"
pg_ctl reload
The log shows:
LOG: received SIGHUP, reloading configuration files
LOG: parameter "recovery_target_xid" cannot be changed without restarting the server
LOG: parameter "recovery_target_time" cannot be changed without restarting the server
LOG: configuration file "postgresql.auto.conf" contains errors; unaffected changes were applied
pg_settings shows:
postgres=# SELECT name, setting, pending_restart FROM pg_settings
WHERE name LIKE 'recovery_target%' AND pending_restart;
name | setting | pending_restart
---------------------+---------+-----------------
recovery_target_time | | t
recovery_target_xid | | t
The db runs fine until the next restart, maybe hours later:
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.
Is it worth a follow-up to report the conflict early and loud?
2. There's an opportunity to provide a better UX by reporting which
flags were set and what the values were, so that the user doesn't have
to search config files or other logs to find this info. For instance,
in the postgresql.auto.conf scenario above, instead of:
DETAIL: At most one of "recovery_target", "recovery_target_lsn",
"recovery_target_name", "recovery_target_time",
"recovery_target_xid" can be set.
The operator could see:
DETAIL: The following recovery target parameters are set:
"recovery_target_time" = "2026-01-01 00:00:00",
"recovery_target_xid" = "700".
HINT: At most one of "recovery_target", "recovery_target_lsn",
"recovery_target_name", "recovery_target_time",
"recovery_target_xid" can be set.
3. 003_recovery_targets.pl:339 currently tests recovery_target_xid's
cleared-then-set behavior. The patch adds the same coverage for the
other four recovery_target_* GUCs.
--
Scott Ray
Attachments:
[application/octet-stream] v1-0001-Report-set-parameters-on-recovery_target-conflict.patch (6.2K, 2-v1-0001-Report-set-parameters-on-recovery_target-conflict.patch)
download | inline diff:
From c61284c824429778ad8f123e22862931d41a2a6d Mon Sep 17 00:00:00 2001
From: Scott Ray <[email protected]>
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)
[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: <M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@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