public inbox for [email protected]
help / color / mirror / Atom feedFrom: JoongHyuk Shin <[email protected]>
To: [email protected]
Cc: Álvaro Herrera <[email protected]>
Cc: Scott Ray <[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: Mon, 29 Jun 2026 13:45:37 +0900
Message-ID: <CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com> (raw)
In-Reply-To: <CAAAe_zD6oOibLjg6zBs5EvNk2C+y0H7wQVp86z_aGDXJg-sMyg@mail.gmail.com>
References: <CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com>
<[email protected]>
<CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com>
<CAAAe_zD6oOibLjg6zBs5EvNk2C+y0H7wQVp86z_aGDXJg-sMyg@mail.gmail.com>
Thanks for the reviews.
v7 attached, reworked along the lines discussed. The assign hooks
no longer touch recoveryTarget; each just stores its own value,
and recoveryTarget is derived once in validateRecoveryParameters()
from the settled recovery_target* settings,
which also rejects setting more than one target.
The v6 review comments are folded in too.
> recovery_target_xid=700 is silently ignored ... target overshoot
> derive recoveryTarget from the settled GUC strings
Done that way. A new test, standby_clobber_clear, sets a competing target
and clears it again; on master that is rejected outright,
while here recovery stops at the xid.
I left out the extra boolean. recovery_target's own string is "immediate"
or empty, so immediate is detected like the other four.
Deriving it from the typed value variables would need the flag,
but those aren't reliable "is set" signals anyway,
since the time value is parsed late and an LSN of 0/0 is valid.
Happy to add it back if you prefer it explicit.
> the legacy extra parentheses in ereport()
Dropped.
--
JH Shin
Attachments:
[application/octet-stream] v7-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch (17.6K, ../CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com/3-v7-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch)
download | inline diff:
From 2c0daee6be5ffdb2ac48e4da687247480ebfb8dc Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <[email protected]>
Date: Sun, 28 Jun 2026 18:25:45 +0900
Subject: [PATCH] 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.
Make the assign hooks store only their own value, and derive
recoveryTarget once in validateRecoveryParameters() from the settled
recovery_target* values, rejecting there a configuration that sets more
than one target.
---
src/backend/access/transam/xlogrecovery.c | 142 ++++++++-----------
src/backend/utils/misc/guc_parameters.dat | 2 -
src/include/utils/guc_hooks.h | 2 -
src/test/recovery/t/003_recovery_targets.pl | 149 ++++++++++++++++----
4 files changed, 183 insertions(+), 112 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c0ae4d3f63f..5c8d019c7bb 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"
@@ -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,14 @@ readRecoverySignalFile(void)
static void
validateRecoveryParameters(void)
{
+ /*
+ * Derive recoveryTarget from the final recovery_target* settings,
+ * rejecting a configuration with more than one of them. This runs before
+ * the early return below so that conflicts are rejected at every startup,
+ * as the assign hooks used to do.
+ */
+ recoveryTarget = DetermineRecoveryTargetType();
+
if (!ArchiveRecoveryRequested)
return;
@@ -4769,30 +4779,59 @@ 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.
+ * Recovery target settings: at most one of the recovery_target* settings may
+ * be set. The assign hooks just store each parameter's own value; the chosen
+ * target and any conflict are derived here instead, from the final settings,
+ * because an assign hook must not raise an error and cannot see sibling GUCs.
+ * validateRecoveryParameters() calls this once after all GUC processing.
*/
-
-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);
+
+ /*
+ * These are all PGC_STRING, so GetConfigOption() returns "" (not NULL)
+ * when unset. The separators and quotes are wrapped in _() so
+ * translators can adapt the list punctuation.
+ */
+#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("multiple recovery targets specified"),
+ errdetail("Only one recovery target can be set. Parameters set: %s.",
+ buf.data),
+ errhint("See pg_settings for the parameter values and where each is set."));
+
+ pfree(buf.data);
+
+ return target;
}
/*
@@ -4809,22 +4848,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 +4879,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;
}
/*
@@ -4891,17 +4905,8 @@ 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;
recoveryTargetName = newval;
- }
- else
- recoveryTarget = RECOVERY_TARGET_UNSET;
}
/*
@@ -4965,22 +4970,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 +5088,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 3c1e6b31bf8..7bc967c629f 100644
--- a/src/backend/utils/misc/guc_parameters.dat
+++ b/src/backend/utils/misc/guc_parameters.dat
@@ -2455,7 +2455,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',
@@ -2492,7 +2491,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/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 307f4fbaefe..1aec17c67bd 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -103,7 +103,6 @@ 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);
@@ -112,7 +111,6 @@ extern bool check_recovery_target_name(char **newval, void **extra,
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..f4d612e4263 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,12 @@ $node_primary->safe_psql('postgres',
# Force archiving of WAL file
$node_primary->safe_psql('postgres', "SELECT pg_switch_wal()");
+# LSN after the final 6000-row insert and WAL switch. The set-then-clear case
+# below has no recovery target and replays all WAL, so it polls on this instead
+# of $lsn5, which would race the 5001-6000 rows.
+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 +174,22 @@ 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. Setting the same
+# parameter twice is allowed (last value wins), and an empty string is a no-op
+# that does not clear another GUC's target. Conflicts are detected at every
+# server start by DetermineRecoveryTargetType().
@recovery_params = (
"recovery_target_name = '$recovery_name'",
@@ -138,31 +198,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 +222,69 @@ 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/multiple recovery targets specified/,
+ 'expected error message logged without recovery.signal');
+like(
+ $logfile_no_signal,
+ qr/Only one recovery target can be set\. Parameters set: "recovery_target_name", "recovery_target_time"/,
+ 'errdetail lists the set parameters in order without recovery.signal');
+unlike(
+ $logfile_no_signal,
+ qr/Parameters set:[^\n]*=/,
+ 'errdetail does not echo parameter values without recovery.signal');
+like(
+ $logfile_no_signal,
+ qr/HINT:.*pg_settings/,
+ 'errhint points to pg_settings without recovery.signal');
+
+# 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
view thread (25+ 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], [email protected], [email protected]
Subject: Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
In-Reply-To: <CACSdjfOFz=i2=YT_2kej-+OA0UTyUO1Hc5kEq-2wqzJcmdR6aw@mail.gmail.com>
* 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