public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v45 1/7] sequential scan for dshash
25+ messages / 9 participants
[nested] [flat]
* [PATCH v45 1/7] sequential scan for dshash
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 25+ messages in thread
From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw)
Dshash did not allow scan the all entries sequentially. This adds the
functionality. The interface is similar but a bit different both from
that of dynahash and simple dshash search functions. One of the most
significant differences is the sequential scan interface of dshash
always needs a call to dshash_seq_term when scan ends. Another is
locking. Dshash holds partition lock when returning an entry,
dshash_seq_next() also holds lock when returning an entry but callers
shouldn't release it, since the lock is essential to continue a
scan. The seqscan interface allows entry deletion while a scan. The
in-scan deletion should be performed by dshash_delete_current().
---
src/backend/lib/dshash.c | 160 ++++++++++++++++++++++++++++++++++++++-
src/include/lib/dshash.h | 22 ++++++
2 files changed, 181 insertions(+), 1 deletion(-)
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index e0c763be32..520bfa0979 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -127,6 +127,10 @@ struct dshash_table
#define NUM_SPLITS(size_log2) \
(size_log2 - DSHASH_NUM_PARTITIONS_LOG2)
+/* How many buckets are there in a given size? */
+#define NUM_BUCKETS(size_log2) \
+ (((size_t) 1) << (size_log2))
+
/* How many buckets are there in each partition at a given size? */
#define BUCKETS_PER_PARTITION(size_log2) \
(((size_t) 1) << NUM_SPLITS(size_log2))
@@ -153,6 +157,10 @@ struct dshash_table
#define BUCKET_INDEX_FOR_PARTITION(partition, size_log2) \
((partition) << NUM_SPLITS(size_log2))
+/* Choose partition based on bucket index. */
+#define PARTITION_FOR_BUCKET_INDEX(bucket_idx, size_log2) \
+ ((bucket_idx) >> NUM_SPLITS(size_log2))
+
/* The head of the active bucket for a given hash value (lvalue). */
#define BUCKET_FOR_HASH(hash_table, hash) \
(hash_table->buckets[ \
@@ -324,7 +332,7 @@ dshash_destroy(dshash_table *hash_table)
ensure_valid_bucket_pointers(hash_table);
/* Free all the entries. */
- size = ((size_t) 1) << hash_table->size_log2;
+ size = NUM_BUCKETS(hash_table->size_log2);
for (i = 0; i < size; ++i)
{
dsa_pointer item_pointer = hash_table->buckets[i];
@@ -592,6 +600,156 @@ dshash_memhash(const void *v, size_t size, void *arg)
return tag_hash(v, size);
}
+/*
+ * dshash_seq_init/_next/_term
+ * Sequentially scan through dshash table and return all the
+ * elements one by one, return NULL when no more.
+ *
+ * dshash_seq_term should always be called when a scan finished.
+ * The caller may delete returned elements midst of a scan by using
+ * dshash_delete_current(). exclusive must be true to delete elements.
+ */
+void
+dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive)
+{
+ status->hash_table = hash_table;
+ status->curbucket = 0;
+ status->nbuckets = 0;
+ status->curitem = NULL;
+ status->pnextitem = InvalidDsaPointer;
+ status->curpartition = -1;
+ status->exclusive = exclusive;
+}
+
+/*
+ * Returns the next element.
+ *
+ * Returned elements are locked and the caller must not explicitly release
+ * it. It is released at the next call to dshash_next().
+ */
+void *
+dshash_seq_next(dshash_seq_status *status)
+{
+ dsa_pointer next_item_pointer;
+
+ if (status->curitem == NULL)
+ {
+ int partition;
+
+ Assert(status->curbucket == 0);
+ Assert(!status->hash_table->find_locked);
+
+ /* first shot. grab the first item. */
+ partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+ LWLockAcquire(PARTITION_LOCK(status->hash_table, partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ status->curpartition = partition;
+
+ /* resize doesn't happen from now until seq scan ends */
+ status->nbuckets =
+ NUM_BUCKETS(status->hash_table->control->size_log2);
+ ensure_valid_bucket_pointers(status->hash_table);
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+ else
+ next_item_pointer = status->pnextitem;
+
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(status->hash_table,
+ status->curpartition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED));
+
+ /* Move to the next bucket if we finished the current bucket */
+ while (!DsaPointerIsValid(next_item_pointer))
+ {
+ int next_partition;
+
+ if (++status->curbucket >= status->nbuckets)
+ {
+ /* all buckets have been scanned. finish. */
+ return NULL;
+ }
+
+ /* Check if move to the next partition */
+ next_partition =
+ PARTITION_FOR_BUCKET_INDEX(status->curbucket,
+ status->hash_table->size_log2);
+
+ if (status->curpartition != next_partition)
+ {
+ /*
+ * Move to the next partition. Lock the next partition then release
+ * the current, not in the reverse order to avoid concurrent
+ * resizing. Avoid dead lock by taking lock in the same order
+ * with resize().
+ */
+ LWLockAcquire(PARTITION_LOCK(status->hash_table,
+ next_partition),
+ status->exclusive ? LW_EXCLUSIVE : LW_SHARED);
+ LWLockRelease(PARTITION_LOCK(status->hash_table,
+ status->curpartition));
+ status->curpartition = next_partition;
+ }
+
+ next_item_pointer = status->hash_table->buckets[status->curbucket];
+ }
+
+ status->curitem =
+ dsa_get_address(status->hash_table->area, next_item_pointer);
+ status->hash_table->find_locked = true;
+ status->hash_table->find_exclusively_locked = status->exclusive;
+
+ /*
+ * The caller may delete the item. Store the next item in case of deletion.
+ */
+ status->pnextitem = status->curitem->next;
+
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
+/*
+ * Terminates the seqscan and release all locks.
+ *
+ * Should be always called when finishing or exiting a seqscan.
+ */
+void
+dshash_seq_term(dshash_seq_status *status)
+{
+ status->hash_table->find_locked = false;
+ status->hash_table->find_exclusively_locked = false;
+
+ if (status->curpartition >= 0)
+ LWLockRelease(PARTITION_LOCK(status->hash_table, status->curpartition));
+}
+
+/* Remove the current entry while a seq scan. */
+void
+dshash_delete_current(dshash_seq_status *status)
+{
+ dshash_table *hash_table = status->hash_table;
+ dshash_table_item *item = status->curitem;
+ size_t partition = PARTITION_FOR_HASH(item->hash);
+
+ Assert(status->exclusive);
+ Assert(hash_table->control->magic == DSHASH_MAGIC);
+ Assert(hash_table->find_locked);
+ Assert(hash_table->find_exclusively_locked);
+ Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition),
+ LW_EXCLUSIVE));
+
+ delete_item(hash_table, item);
+}
+
+/* Get the current entry while a seq scan. */
+void *
+dshash_get_current(dshash_seq_status *status)
+{
+ return ENTRY_FROM_ITEM(status->curitem);
+}
+
/*
* Print debugging information about the internal state of the hash table to
* stderr. The caller must hold no partition locks.
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index c069ec9de7..a6ea377173 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -59,6 +59,21 @@ typedef struct dshash_parameters
struct dshash_table_item;
typedef struct dshash_table_item dshash_table_item;
+/*
+ * Sequential scan state. The detail is exposed to let users know the storage
+ * size but it should be considered as an opaque type by callers.
+ */
+typedef struct dshash_seq_status
+{
+ dshash_table *hash_table; /* dshash table working on */
+ int curbucket; /* bucket number we are at */
+ int nbuckets; /* total number of buckets in the dshash */
+ dshash_table_item *curitem; /* item we are currently at */
+ dsa_pointer pnextitem; /* dsa-pointer to the next item */
+ int curpartition; /* partition number we are at */
+ bool exclusive; /* locking mode */
+} dshash_seq_status;
+
/* Creating, sharing and destroying from hash tables. */
extern dshash_table *dshash_create(dsa_area *area,
const dshash_parameters *params,
@@ -80,6 +95,13 @@ extern bool dshash_delete_key(dshash_table *hash_table, const void *key);
extern void dshash_delete_entry(dshash_table *hash_table, void *entry);
extern void dshash_release_lock(dshash_table *hash_table, void *entry);
+/* seq scan support */
+extern void dshash_seq_init(dshash_seq_status *status, dshash_table *hash_table,
+ bool exclusive);
+extern void *dshash_seq_next(dshash_seq_status *status);
+extern void dshash_seq_term(dshash_seq_status *status);
+extern void dshash_delete_current(dshash_seq_status *status);
+extern void *dshash_get_current(dshash_seq_status *status);
/* Convenience hash and compare functions wrapping memcmp and tag_hash. */
extern int dshash_memcmp(const void *a, const void *b, size_t size, void *arg);
extern dshash_hash dshash_memhash(const void *v, size_t size, void *arg);
--
2.27.0
----Next_Part(Fri_Jan__8_10_24_34_2021_185)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v45-0002-Add-conditional-lock-feature-to-dshash.patch"
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
@ 2026-05-31 21:11 Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Scott Ray @ 2026-05-31 21:11 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
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, ../../M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io/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, ../../M5a4v6-PJIZSNQlLhGy-GgBPCXNkrW3OnbESqTpaXh2TyZl4TsMyY_wPK-chiMOoZ4mr9Dm7vuqDSVmNeMKfZYNYlI5PxoQyC6iqWZ9MTAk=@scottray.io/3-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
@ 2026-06-04 05:41 ` JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-04 05:41 UTC (permalink / raw)
To: [email protected]; +Cc: Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
Thanks for the patch.
I went through 'v1-0001-Report-....patch'
and have a few observations to share.
* 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.
On the reload trap:
I reproduced this on master and confirmed it's there exactly as you noted.
ALTER SYSTEM doesn't trigger the assign hook;
it just writes to postgresql.auto.conf,
so the trap window is intrinsic to PGC_POSTMASTER + ALTER SYSTEM.
A separate follow-up patch in the reload path feels natural.
--
JH Shin
On Mon, Jun 1, 2026 at 6:11 AM Scott Ray <[email protected]> wrote:
> 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
>
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-06 20:10 ` Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Scott Ray @ 2026-06-06 20:10 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
> * 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, ../../0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io/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, ../../0WrsV1VojwurDot6hdS0norm0nK9QFDk0KSLqBcXK4Xz2b_sMuIdE0zyiXP0p1hTRD7Hwz3C8AQouGyV7yZMhf5joz3zUpQee3l_pWPs4dk=@scottray.io/3-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
@ 2026-06-07 10:30 ` JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-07 10:30 UTC (permalink / raw)
To: Scott Ray <[email protected]>; +Cc: Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
On "=" vs "set to": I'd stay with "=".
The list is assembled in appendStringInfo and ends up in the dynamic %s,
so prose like "set to" there never goes through gettext
and would print in English even under a translated lc_messages.
"=" is punctuation, so it sidesteps that.
PG already uses this form in the RI detail,
e.g. Key (%s)=(%s) in ri_triggers.c.
That said, I'm not sure the currently-set list belongs in the errdetail at
all,
since an operator can read the values back from the configuration.
I'd be interested in the committer's view on whether it is worth adding.
--
JH Shin
On Sun, Jun 7, 2026 at 5:10 AM Scott Ray <[email protected]> wrote:
> > * 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
>
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-07 15:44 ` Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Álvaro Herrera @ 2026-06-07 15:44 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
On 2026-Jun-07, JoongHyuk Shin wrote:
> On "=" vs "set to": I'd stay with "=".
> The list is assembled in appendStringInfo and ends up in the dynamic %s,
> so prose like "set to" there never goes through gettext
> and would print in English even under a translated lc_messages.
> "=" is punctuation, so it sidesteps that.
Agreed on using =, although ...
> That said, I'm not sure the currently-set list belongs in the errdetail at
> all,
> since an operator can read the values back from the configuration.
> I'd be interested in the committer's view on whether it is worth adding.
It may be enough to list which ones are set, without listing their values.
Those can be obtained easily from pg_settings, which can be mentioned in
errhint -- useful also to figure out exactly _where_ they are set (e.g.,
in an include file, postgresql.auto.conf, and so on.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
@ 2026-06-21 09:27 ` JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-21 09:27 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
Thanks for the reviews.
v5 attached.
The errdetail now lists which recovery_target_* parameters are actually set,
instead of the full candidate list.
This follows Scott's idea to surface the set targets;
following Álvaro, the values are dropped
and a new errhint points to pg_settings for them and their sources.
I kept the "which ones are set" list in errdetail rather than errhint,
it states the current configuration, which reads as detail,
while the errhint carries the actionable pg_settings pointer.
--
JH Shin
On Mon, Jun 8, 2026 at 12:44 AM Álvaro Herrera <[email protected]> wrote:
> On 2026-Jun-07, JoongHyuk Shin wrote:
>
> > On "=" vs "set to": I'd stay with "=".
> > The list is assembled in appendStringInfo and ends up in the dynamic %s,
> > so prose like "set to" there never goes through gettext
> > and would print in English even under a translated lc_messages.
> > "=" is punctuation, so it sidesteps that.
>
> Agreed on using =, although ...
>
> > That said, I'm not sure the currently-set list belongs in the errdetail
> at
> > all,
> > since an operator can read the values back from the configuration.
> > I'd be interested in the committer's view on whether it is worth adding.
>
> It may be enough to list which ones are set, without listing their values.
> Those can be obtained easily from pg_settings, which can be mentioned in
> errhint -- useful also to figure out exactly _where_ they are set (e.g.,
> in an include file, postgresql.auto.conf, and so on.)
>
> --
> Álvaro Herrera Breisgau, Deutschland —
> https://www.EnterpriseDB.com/
>
Attachments:
[application/octet-stream] v5-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC-as.patch (22.5K, ../../CACSdjfN7uYLS-+wyBbvZ6rmkSNuPm9H8Y+xdd_aSr-kNG_xu3w@mail.gmail.com/3-v5-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC-as.patch)
download | inline diff:
From 3638a7ba5037c46e0106989a6679906c03a2fc6b Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <[email protected]>
Date: Sun, 21 Jun 2026 17:45:25 +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 a new CheckRecoveryTargetConflicts()
function, called from validateRecoveryParameters() before its early
return for !ArchiveRecoveryRequested. The check therefore runs at
every startup regardless of recovery mode, preserving the existing
behavior of detecting misconfiguration at server start time rather
than only when recovery.signal is added.
In each assign hook's empty-string branch, replace the unconditional
"recoveryTarget = RECOVERY_TARGET_UNSET" assignment with a narrower
"if (recoveryTarget == MY_TYPE)" form. Empty strings for an unrelated
recovery_target_* GUC then leave another GUC's already-set target
intact, while still preserving the documented "last value wins"
reassignment semantics for the same GUC.
When a conflict is detected, the errdetail now lists which
recovery_target_* parameters are actually set, rather than the full
list of candidate names, and an errhint points to pg_settings for their
values and where each is configured.
---
src/backend/access/transam/xlogrecovery.c | 159 +++++++++----
src/test/recovery/t/003_recovery_targets.pl | 251 +++++++++++++++++++-
2 files changed, 361 insertions(+), 49 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 4d61795b483..dc8b46a6c81 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 void CheckRecoveryTargetConflicts(void);
static bool read_backup_label(XLogRecPtr *checkPointLoc,
TimeLineID *backupLabelTLI,
bool *backupEndRequired, bool *backupFromStandby);
@@ -1067,6 +1069,8 @@ readRecoverySignalFile(void)
static void
validateRecoveryParameters(void)
{
+ CheckRecoveryTargetConflicts();
+
if (!ArchiveRecoveryRequested)
return;
@@ -1144,6 +1148,94 @@ validateRecoveryParameters(void)
}
}
+/*
+ * CheckRecoveryTargetConflicts
+ *
+ * Validate that at most one of the recovery_target_* GUCs is set to a
+ * non-empty value. This is called from validateRecoveryParameters() at every
+ * server startup, regardless of whether archive recovery is requested, so
+ * misconfiguration is detected at server start time rather than later when
+ * recovery.signal is added.
+ *
+ * The check is a separate function rather than inlined into
+ * validateRecoveryParameters() because it intentionally runs even when no
+ * recovery is requested, while the rest of validateRecoveryParameters() is
+ * recovery-mode-only. Keeping it as a named function makes that separation
+ * explicit.
+ *
+ * The check used to live in the assign hooks of the recovery_target_* GUCs
+ * (calling ereport(ERROR) on conflict), which violated guc.c's contract that
+ * 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, it must be added to the
+ * list of GetConfigOption() checks below; the errdetail builds its list of set
+ * parameters dynamically, so it needs no change.
+ */
+static void
+CheckRecoveryTargetConflicts(void)
+{
+ int ntargets = 0;
+ const char *val;
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+
+ /*
+ * These GUCs are all PGC_STRING, so GetConfigOption() returns "" (not
+ * NULL) when a parameter is unset. Collect the name of each one that is
+ * set into buf, separated by ", ", for use in the errdetail below.
+ */
+ val = GetConfigOption("recovery_target", false, false);
+ if (val[0] != '\0')
+ {
+ ntargets++;
+ if (buf.len > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, "\"recovery_target\"");
+ }
+ val = GetConfigOption("recovery_target_lsn", false, false);
+ if (val[0] != '\0')
+ {
+ ntargets++;
+ if (buf.len > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, "\"recovery_target_lsn\"");
+ }
+ val = GetConfigOption("recovery_target_name", false, false);
+ if (val[0] != '\0')
+ {
+ ntargets++;
+ if (buf.len > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, "\"recovery_target_name\"");
+ }
+ val = GetConfigOption("recovery_target_time", false, false);
+ if (val[0] != '\0')
+ {
+ ntargets++;
+ if (buf.len > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, "\"recovery_target_time\"");
+ }
+ val = GetConfigOption("recovery_target_xid", false, false);
+ if (val[0] != '\0')
+ {
+ ntargets++;
+ if (buf.len > 0)
+ appendStringInfoString(&buf, ", ");
+ appendStringInfoString(&buf, "\"recovery_target_xid\"");
+ }
+
+ if (ntargets > 1)
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("multiple recovery targets specified"),
+ errdetail("Recovery targets %s are set, but only one can be set.",
+ buf.data),
+ errhint("See pg_settings for the parameter values and where each is set.")));
+}
+
/*
* read_backup_label: check to see if a backup_label file is present
*
@@ -4769,32 +4861,27 @@ 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
+ * Recovery target settings: At most one of the several recovery_target*
+ * settings may be set to a non-empty value. 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.
+ * An empty string for any of these GUCs is treated as "not set", equivalent
+ * to the GUC's default; an empty value cannot clobber another GUC's
+ * already-set target. Conflicts between multiple non-empty settings are
+ * detected in CheckRecoveryTargetConflicts(), called from
+ * validateRecoveryParameters() at every startup.
*
- * 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.
+ * Each assign hook clears recoveryTarget only when its own GUC is reassigned
+ * to an empty string after the same GUC was previously assigned a non-empty
+ * value, e.g.
+ * postgres -c recovery_target_xid=700 -c recovery_target_xid=
+ * (postgresql.conf collapses duplicate keys so only the last value reaches
+ * the assign hook; this same-parameter set-then-clear case only arises from
+ * -c). The clear is restricted to the hook's own target type so that an
+ * empty value for one recovery_target_* GUC cannot clobber another GUC's
+ * already-set target.
*/
-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
*/
@@ -4815,13 +4902,9 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4856,16 +4939,12 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_LSN)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4891,16 +4970,12 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_NAME)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4971,13 +5046,9 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_TIME)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -5099,15 +5170,11 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_XID)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 047eb13293a..6e8a2c557c9 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,13 @@ $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. Used by the
+# set-then-cleared scenarios below where recovery has no target and must
+# replay all archived WAL; polling on $lsn5 would race against 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 +175,24 @@ 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 not allowed. Setting the same
+# parameter multiple times is allowed (the last value wins, per ProcessConfigFile
+# duplicate handling). An empty string for a recovery_target_* GUC is treated
+# as the GUC's default and is a no-op; it does not clear any other already-set
+# target. Conflict detection runs in CheckRecoveryTargetConflicts() at every
+# server start, regardless of whether recovery is requested.
@recovery_params = (
"recovery_target_name = '$recovery_name'",
@@ -159,6 +222,21 @@ like(
$logfile,
qr/multiple recovery targets specified/,
'multiple conflicting settings');
+# errdetail lists exactly the parameters that are set, in GUC-check order
+# (recovery_target_name is checked before recovery_target_time).
+like(
+ $logfile,
+ qr/Recovery targets "recovery_target_name", "recovery_target_time" are set/,
+ 'errdetail lists the set parameters in order');
+# The parameter values must not be echoed (only the names are listed).
+unlike(
+ $logfile,
+ qr/are set[^\n]*=/,
+ 'errdetail does not echo parameter values');
+like(
+ $logfile,
+ qr/HINT:.*pg_settings/,
+ 'errhint points to pg_settings');
# Check behavior when recovery ends before target is reached
@@ -190,6 +268,173 @@ like(
qr/FATAL: .* recovery ended before configured recovery target was reached/,
'recovery end before target reached is a fatal error');
+# Conflicting recovery targets are rejected at every startup, regardless of
+# whether recovery.signal is present. Use a throwaway cluster initialized
+# from the existing backup so we can leave conflicting GUCs in its
+# postgresql.conf without polluting the primary used by later tests.
+# init_from_backup is called without has_restoring, so no recovery.signal
+# is created and the cluster would normally start as a plain primary; the
+# conflicting recovery_target_* GUCs must be rejected anyway.
+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/Recovery targets "recovery_target_name", "recovery_target_time" are set/,
+ 'errdetail lists the set parameters in order without recovery.signal');
+unlike(
+ $logfile_no_signal,
+ qr/are 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');
+
+# Conflict involving the bare recovery_target (no suffix): it is checked first,
+# so its name appears at the head of the list. The list entries are quoted, so
+# the regex anchors on the closing quote to ensure "recovery_target" does not
+# match a prefix of "recovery_target_xid".
+my $node_bare = PostgreSQL::Test::Cluster->new('multi_target_bare');
+$node_bare->init_from_backup($node_primary, 'my_backup');
+$node_bare->append_conf('postgresql.conf',
+ "recovery_target = 'immediate'\nrecovery_target_xid = '$recovery_txid'");
+
+my $res_bare = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_bare->data_dir,
+ '--log' => $node_bare->logfile,
+ 'start',
+ ]);
+ok(!$res_bare,
+ 'server fails to start with bare recovery_target conflict');
+
+my $logfile_bare = slurp_file($node_bare->logfile());
+like(
+ $logfile_bare,
+ qr/Recovery targets "recovery_target", "recovery_target_xid" are set/,
+ 'errdetail lists the bare recovery_target first, without prefix mismatch');
+
+# Three conflicting targets: exercises the ", " separator between more than two
+# entries (no trailing separator after the last entry).
+my $node_three = PostgreSQL::Test::Cluster->new('multi_target_three');
+$node_three->init_from_backup($node_primary, 'my_backup');
+$node_three->append_conf('postgresql.conf',
+ "recovery_target_name = '$recovery_name'\n"
+ . "recovery_target_time = '$recovery_time'\n"
+ . "recovery_target_xid = '$recovery_txid'");
+
+my $res_three = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_three->data_dir,
+ '--log' => $node_three->logfile,
+ 'start',
+ ]);
+ok(!$res_three,
+ 'server fails to start with three conflicting recovery targets');
+
+my $logfile_three = slurp_file($node_three->logfile());
+like(
+ $logfile_three,
+ qr/Recovery targets "recovery_target_name", "recovery_target_time", "recovery_target_xid" are set/,
+ 'errdetail lists three set parameters with correct separators');
+
+# 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
+# "set then unset" form of GUC reassignment; the assign hook clears its own
+# type when the new value is empty. postgresql.conf cannot express duplicate
+# keys (ProcessConfigFile collapses them), so the postmaster command line is
+# used via "pg_ctl --options". Each of the five recovery_target_* assign
+# hooks is exercised once.
+#
+# Note: GUC values below are passed unquoted on the command line. Windows
+# cmd.exe does not strip single quotes the way POSIX shells do, so quoted
+# values would reach the postmaster verbatim and be rejected. recovery_time
+# from now() contains a space; we replace it with the ISO 8601 T separator
+# so the value is a single shell token without quoting.
+my $recovery_time_t = $recovery_time;
+$recovery_time_t =~ s/ /T/;
+
+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);
+
+test_recovery_standby_with_options(
+ 'recovery_target_time set then cleared',
+ 'standby_time_set_clear', $node_primary,
+ "-c recovery_target_time=$recovery_time_t -c recovery_target_time=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target_name set then cleared',
+ 'standby_name_set_clear', $node_primary,
+ "-c recovery_target_name=$recovery_name -c recovery_target_name=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target_lsn set then cleared',
+ 'standby_lsn_set_clear', $node_primary,
+ "-c recovery_target_lsn=$recovery_lsn -c recovery_target_lsn=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target set then cleared',
+ 'standby_immediate_set_clear', $node_primary,
+ "-c recovery_target=immediate -c recovery_target=",
+ "6000", $lsn6);
+
+# Same-GUC empty-then-set sanity: assigning an empty string and then a
+# non-empty value to the same GUC must end with the target set. The empty
+# value seen first must not poison a later non-empty assignment.
+my $node_xid_clear_set =
+ PostgreSQL::Test::Cluster->new('standby_xid_clear_set');
+$node_xid_clear_set->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+
+my $res_xid_clear_set = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_xid_clear_set->data_dir,
+ '--log' => $node_xid_clear_set->logfile,
+ '--options' =>
+ "-c recovery_target_xid= -c recovery_target_xid=$recovery_txid",
+ 'start',
+ ]);
+ok($res_xid_clear_set,
+ 'server starts with recovery_target_xid cleared then set');
+
+$node_xid_clear_set->poll_query_until('postgres',
+ "SELECT '$lsn2'::pg_lsn <= pg_last_wal_replay_lsn()")
+ or die "Timed out while waiting for standby to catch up";
+my $count_xid_clear_set = $node_xid_clear_set->safe_psql('postgres',
+ "SELECT count(*) FROM tab_int");
+is($count_xid_clear_set, "2000",
+ 'recovery_target_xid honored when cleared then set');
+$node_xid_clear_set->teardown_node;
+
# Invalid recovery_target_timeline tests
my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
"ALTER SYSTEM SET recovery_target_timeline TO 'bogus'");
--
2.52.0
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-21 12:31 ` Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Álvaro Herrera @ 2026-06-21 12:31 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
Hello,
On 2026-Jun-21, JoongHyuk Shin wrote:
> The errdetail now lists which recovery_target_* parameters are actually set,
> instead of the full candidate list.
> This follows Scott's idea to surface the set targets;
> following Álvaro, the values are dropped
> and a new errhint points to pg_settings for them and their sources.
>
> I kept the "which ones are set" list in errdetail rather than errhint,
> it states the current configuration, which reads as detail,
> while the errhint carries the actionable pg_settings pointer.
Please see https://postgr.es/c/3692a622d3fd for more on translatable
message construction. You should end up with a translatable string in
a _() call like
_(", \"%s\"")
and the GUC names in a separate string in each case.
Maybe you can make this a local macro to avoid repetitive coding,
#define considerAndComplainAboutGUC(gucname, buf) \
do { \
val = GetConfigOption(gucname, false, false); \
if (val[0] != '\0') \
{ \
ntargets++; \
if (buf.len == 0) \
appendStringInfoString(&buf, _("\"%s\""), gucname); \
else \
appendStringInfoString(&buf, _(", \"%s\""), gucname); \
} \
} while (0)
considerAndComplainAboutGUC("recovery_target", buf);
considerAndComplainAboutGUC("recovery_target_lsn", buf);
and so on. (Of course, you should choose a less stupid macro name, but
you get my meaning.)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"... In accounting terms this makes perfect sense. To rational humans, it
is insane. Welcome to IBM." (Robert X. Cringely)
https://www.cringely.com/2015/06/03/autodesks-john-walker-explained-hp-and-ibm-in-1991/
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
@ 2026-06-22 04:37 ` JoongHyuk Shin <[email protected]>
2026-06-25 08:00 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
0 siblings, 2 replies; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-22 04:37 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
Thanks for the review.
v6 attached.
The set-parameter list in the errdetail now wraps the separator and quotes
in _()
so the punctuation is translatable, following 3692a622d3fd.
The five GetConfigOption() checks are now a local macro, as suggested.
--
JH Shin
On Sun, Jun 21, 2026 at 9:32 PM Álvaro Herrera <[email protected]> wrote:
> Hello,
>
> On 2026-Jun-21, JoongHyuk Shin wrote:
>
> > The errdetail now lists which recovery_target_* parameters are actually
> set,
> > instead of the full candidate list.
> > This follows Scott's idea to surface the set targets;
> > following Álvaro, the values are dropped
> > and a new errhint points to pg_settings for them and their sources.
> >
> > I kept the "which ones are set" list in errdetail rather than errhint,
> > it states the current configuration, which reads as detail,
> > while the errhint carries the actionable pg_settings pointer.
>
> Please see https://postgr.es/c/3692a622d3fd for more on translatable
> message construction. You should end up with a translatable string in
> a _() call like
> _(", \"%s\"")
> and the GUC names in a separate string in each case.
>
> Maybe you can make this a local macro to avoid repetitive coding,
>
> #define considerAndComplainAboutGUC(gucname, buf) \
> do { \
> val = GetConfigOption(gucname, false, false); \
> if (val[0] != '\0') \
> { \
> ntargets++; \
> if (buf.len == 0) \
> appendStringInfoString(&buf, _("\"%s\""), gucname); \
> else \
> appendStringInfoString(&buf, _(", \"%s\""), gucname); \
> } \
> } while (0)
>
> considerAndComplainAboutGUC("recovery_target", buf);
> considerAndComplainAboutGUC("recovery_target_lsn", buf);
> and so on. (Of course, you should choose a less stupid macro name, but
> you get my meaning.)
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "... In accounting terms this makes perfect sense. To rational humans, it
> is insane. Welcome to IBM." (Robert X. Cringely)
>
> https://www.cringely.com/2015/06/03/autodesks-john-walker-explained-hp-and-ibm-in-1991/
>
Attachments:
[application/octet-stream] v6-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch (22.0K, ../../CACSdjfMt5obQc0QCX8E+v-pxROWFQxmeW6Ki9X16F60D3tSJgg@mail.gmail.com/3-v6-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch)
download | inline diff:
From 7f05600fb76699a09daa671ddf4d5c85d1c944d9 Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <[email protected]>
Date: Sun, 21 Jun 2026 17:45:25 +0900
Subject: [PATCH v6] 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 a new CheckRecoveryTargetConflicts()
function, called from validateRecoveryParameters() before its early
return for !ArchiveRecoveryRequested. The check therefore runs at
every startup regardless of recovery mode, preserving the existing
behavior of detecting misconfiguration at server start time rather
than only when recovery.signal is added.
In each assign hook's empty-string branch, replace the unconditional
"recoveryTarget = RECOVERY_TARGET_UNSET" assignment with a narrower
"if (recoveryTarget == MY_TYPE)" form. Empty strings for an unrelated
recovery_target_* GUC then leave another GUC's already-set target
intact, while still preserving the documented "last value wins"
reassignment semantics for the same GUC.
When a conflict is detected, the errdetail now lists which
recovery_target_* parameters are actually set, rather than the full
list of candidate names, and an errhint points to pg_settings for their
values and where each is configured.
---
src/backend/access/transam/xlogrecovery.c | 140 +++++++----
src/test/recovery/t/003_recovery_targets.pl | 251 +++++++++++++++++++-
2 files changed, 342 insertions(+), 49 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 4d61795b483..32d209b3957 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 void CheckRecoveryTargetConflicts(void);
static bool read_backup_label(XLogRecPtr *checkPointLoc,
TimeLineID *backupLabelTLI,
bool *backupEndRequired, bool *backupFromStandby);
@@ -1067,6 +1069,8 @@ readRecoverySignalFile(void)
static void
validateRecoveryParameters(void)
{
+ CheckRecoveryTargetConflicts();
+
if (!ArchiveRecoveryRequested)
return;
@@ -1144,6 +1148,75 @@ validateRecoveryParameters(void)
}
}
+/*
+ * CheckRecoveryTargetConflicts
+ *
+ * Validate that at most one of the recovery_target_* GUCs is set to a
+ * non-empty value. This is called from validateRecoveryParameters() at every
+ * server startup, regardless of whether archive recovery is requested, so
+ * misconfiguration is detected at server start time rather than later when
+ * recovery.signal is added.
+ *
+ * The check is a separate function rather than inlined into
+ * validateRecoveryParameters() because it intentionally runs even when no
+ * recovery is requested, while the rest of validateRecoveryParameters() is
+ * recovery-mode-only. Keeping it as a named function makes that separation
+ * explicit.
+ *
+ * The check used to live in the assign hooks of the recovery_target_* GUCs
+ * (calling ereport(ERROR) on conflict), which violated guc.c's contract that
+ * 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, it must be added to the
+ * list of ADD_TARGET_IF_SET() calls below; the errdetail builds its list of set
+ * parameters dynamically, so it needs no change.
+ */
+static void
+CheckRecoveryTargetConflicts(void)
+{
+ int ntargets = 0;
+ const char *val;
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+
+ /*
+ * These GUCs are all PGC_STRING, so GetConfigOption() returns "" (not
+ * NULL) when a parameter is unset. Collect the name of each one that is
+ * set into buf for use in the errdetail below. The separator and the
+ * quotes are wrapped in _() so translators can adapt the list punctuation
+ * to their locale.
+ */
+#define ADD_TARGET_IF_SET(gucname) \
+ do { \
+ val = GetConfigOption(gucname, false, false); \
+ if (val[0] != '\0') \
+ { \
+ ntargets++; \
+ if (buf.len == 0) \
+ appendStringInfo(&buf, _("\"%s\""), gucname); \
+ else \
+ appendStringInfo(&buf, _(", \"%s\""), gucname); \
+ } \
+ } while (0)
+
+ ADD_TARGET_IF_SET("recovery_target");
+ ADD_TARGET_IF_SET("recovery_target_lsn");
+ ADD_TARGET_IF_SET("recovery_target_name");
+ ADD_TARGET_IF_SET("recovery_target_time");
+ ADD_TARGET_IF_SET("recovery_target_xid");
+#undef ADD_TARGET_IF_SET
+
+ if (ntargets > 1)
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("multiple recovery targets specified"),
+ errdetail("Recovery targets %s are set, but only one can be set.",
+ buf.data),
+ errhint("See pg_settings for the parameter values and where each is set.")));
+}
+
/*
* read_backup_label: check to see if a backup_label file is present
*
@@ -4769,32 +4842,27 @@ 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
+ * Recovery target settings: At most one of the several recovery_target*
+ * settings may be set to a non-empty value. 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.
+ * An empty string for any of these GUCs is treated as "not set", equivalent
+ * to the GUC's default; an empty value cannot clobber another GUC's
+ * already-set target. Conflicts between multiple non-empty settings are
+ * detected in CheckRecoveryTargetConflicts(), called from
+ * validateRecoveryParameters() at every startup.
*
- * 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.
+ * Each assign hook clears recoveryTarget only when its own GUC is reassigned
+ * to an empty string after the same GUC was previously assigned a non-empty
+ * value, e.g.
+ * postgres -c recovery_target_xid=700 -c recovery_target_xid=
+ * (postgresql.conf collapses duplicate keys so only the last value reaches
+ * the assign hook; this same-parameter set-then-clear case only arises from
+ * -c). The clear is restricted to the hook's own target type so that an
+ * empty value for one recovery_target_* GUC cannot clobber another GUC's
+ * already-set target.
*/
-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
*/
@@ -4815,13 +4883,9 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4856,16 +4920,12 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_LSN)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4891,16 +4951,12 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_NAME)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -4971,13 +5027,9 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_TIME)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
@@ -5099,15 +5151,11 @@ 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
+ else if (recoveryTarget == RECOVERY_TARGET_XID)
recoveryTarget = RECOVERY_TARGET_UNSET;
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 047eb13293a..6e8a2c557c9 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,13 @@ $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. Used by the
+# set-then-cleared scenarios below where recovery has no target and must
+# replay all archived WAL; polling on $lsn5 would race against 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 +175,24 @@ 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 not allowed. Setting the same
+# parameter multiple times is allowed (the last value wins, per ProcessConfigFile
+# duplicate handling). An empty string for a recovery_target_* GUC is treated
+# as the GUC's default and is a no-op; it does not clear any other already-set
+# target. Conflict detection runs in CheckRecoveryTargetConflicts() at every
+# server start, regardless of whether recovery is requested.
@recovery_params = (
"recovery_target_name = '$recovery_name'",
@@ -159,6 +222,21 @@ like(
$logfile,
qr/multiple recovery targets specified/,
'multiple conflicting settings');
+# errdetail lists exactly the parameters that are set, in GUC-check order
+# (recovery_target_name is checked before recovery_target_time).
+like(
+ $logfile,
+ qr/Recovery targets "recovery_target_name", "recovery_target_time" are set/,
+ 'errdetail lists the set parameters in order');
+# The parameter values must not be echoed (only the names are listed).
+unlike(
+ $logfile,
+ qr/are set[^\n]*=/,
+ 'errdetail does not echo parameter values');
+like(
+ $logfile,
+ qr/HINT:.*pg_settings/,
+ 'errhint points to pg_settings');
# Check behavior when recovery ends before target is reached
@@ -190,6 +268,173 @@ like(
qr/FATAL: .* recovery ended before configured recovery target was reached/,
'recovery end before target reached is a fatal error');
+# Conflicting recovery targets are rejected at every startup, regardless of
+# whether recovery.signal is present. Use a throwaway cluster initialized
+# from the existing backup so we can leave conflicting GUCs in its
+# postgresql.conf without polluting the primary used by later tests.
+# init_from_backup is called without has_restoring, so no recovery.signal
+# is created and the cluster would normally start as a plain primary; the
+# conflicting recovery_target_* GUCs must be rejected anyway.
+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/Recovery targets "recovery_target_name", "recovery_target_time" are set/,
+ 'errdetail lists the set parameters in order without recovery.signal');
+unlike(
+ $logfile_no_signal,
+ qr/are 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');
+
+# Conflict involving the bare recovery_target (no suffix): it is checked first,
+# so its name appears at the head of the list. The list entries are quoted, so
+# the regex anchors on the closing quote to ensure "recovery_target" does not
+# match a prefix of "recovery_target_xid".
+my $node_bare = PostgreSQL::Test::Cluster->new('multi_target_bare');
+$node_bare->init_from_backup($node_primary, 'my_backup');
+$node_bare->append_conf('postgresql.conf',
+ "recovery_target = 'immediate'\nrecovery_target_xid = '$recovery_txid'");
+
+my $res_bare = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_bare->data_dir,
+ '--log' => $node_bare->logfile,
+ 'start',
+ ]);
+ok(!$res_bare,
+ 'server fails to start with bare recovery_target conflict');
+
+my $logfile_bare = slurp_file($node_bare->logfile());
+like(
+ $logfile_bare,
+ qr/Recovery targets "recovery_target", "recovery_target_xid" are set/,
+ 'errdetail lists the bare recovery_target first, without prefix mismatch');
+
+# Three conflicting targets: exercises the ", " separator between more than two
+# entries (no trailing separator after the last entry).
+my $node_three = PostgreSQL::Test::Cluster->new('multi_target_three');
+$node_three->init_from_backup($node_primary, 'my_backup');
+$node_three->append_conf('postgresql.conf',
+ "recovery_target_name = '$recovery_name'\n"
+ . "recovery_target_time = '$recovery_time'\n"
+ . "recovery_target_xid = '$recovery_txid'");
+
+my $res_three = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_three->data_dir,
+ '--log' => $node_three->logfile,
+ 'start',
+ ]);
+ok(!$res_three,
+ 'server fails to start with three conflicting recovery targets');
+
+my $logfile_three = slurp_file($node_three->logfile());
+like(
+ $logfile_three,
+ qr/Recovery targets "recovery_target_name", "recovery_target_time", "recovery_target_xid" are set/,
+ 'errdetail lists three set parameters with correct separators');
+
+# 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
+# "set then unset" form of GUC reassignment; the assign hook clears its own
+# type when the new value is empty. postgresql.conf cannot express duplicate
+# keys (ProcessConfigFile collapses them), so the postmaster command line is
+# used via "pg_ctl --options". Each of the five recovery_target_* assign
+# hooks is exercised once.
+#
+# Note: GUC values below are passed unquoted on the command line. Windows
+# cmd.exe does not strip single quotes the way POSIX shells do, so quoted
+# values would reach the postmaster verbatim and be rejected. recovery_time
+# from now() contains a space; we replace it with the ISO 8601 T separator
+# so the value is a single shell token without quoting.
+my $recovery_time_t = $recovery_time;
+$recovery_time_t =~ s/ /T/;
+
+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);
+
+test_recovery_standby_with_options(
+ 'recovery_target_time set then cleared',
+ 'standby_time_set_clear', $node_primary,
+ "-c recovery_target_time=$recovery_time_t -c recovery_target_time=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target_name set then cleared',
+ 'standby_name_set_clear', $node_primary,
+ "-c recovery_target_name=$recovery_name -c recovery_target_name=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target_lsn set then cleared',
+ 'standby_lsn_set_clear', $node_primary,
+ "-c recovery_target_lsn=$recovery_lsn -c recovery_target_lsn=",
+ "6000", $lsn6);
+
+test_recovery_standby_with_options(
+ 'recovery_target set then cleared',
+ 'standby_immediate_set_clear', $node_primary,
+ "-c recovery_target=immediate -c recovery_target=",
+ "6000", $lsn6);
+
+# Same-GUC empty-then-set sanity: assigning an empty string and then a
+# non-empty value to the same GUC must end with the target set. The empty
+# value seen first must not poison a later non-empty assignment.
+my $node_xid_clear_set =
+ PostgreSQL::Test::Cluster->new('standby_xid_clear_set');
+$node_xid_clear_set->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+
+my $res_xid_clear_set = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_xid_clear_set->data_dir,
+ '--log' => $node_xid_clear_set->logfile,
+ '--options' =>
+ "-c recovery_target_xid= -c recovery_target_xid=$recovery_txid",
+ 'start',
+ ]);
+ok($res_xid_clear_set,
+ 'server starts with recovery_target_xid cleared then set');
+
+$node_xid_clear_set->poll_query_until('postgres',
+ "SELECT '$lsn2'::pg_lsn <= pg_last_wal_replay_lsn()")
+ or die "Timed out while waiting for standby to catch up";
+my $count_xid_clear_set = $node_xid_clear_set->safe_psql('postgres',
+ "SELECT count(*) FROM tab_int");
+is($count_xid_clear_set, "2000",
+ 'recovery_target_xid honored when cleared then set');
+$node_xid_clear_set->teardown_node;
+
# Invalid recovery_target_timeline tests
my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
"ALTER SYSTEM SET recovery_target_timeline TO 'bogus'");
--
2.52.0
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-25 08:00 ` Michael Paquier <[email protected]>
2026-06-26 07:15 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Kyotaro Horiguchi <[email protected]>
1 sibling, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2026-06-25 08:00 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Scott Ray <[email protected]>; Fujii Masao <[email protected]>; [email protected]
On Mon, Jun 22, 2026 at 01:37:27PM +0900, JoongHyuk Shin wrote:
> Thanks for the review.
Please avoid top-posting when replying on the lists. This breaks the
logical flow of the thread.
> The five GetConfigOption() checks are now a local macro, as suggested.
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("multiple recovery targets specified"),
+ errdetail("Recovery targets %s are set, but only one can be set.",
+ buf.data),
Is this errdetail() the optimal choice, though? It feels a bit odd to
have that mid-sentence. Perhaps we should use a colon to separate the
list of parameters, say simply:
"Parameters set are: %s."
The trick of Alvaro to make the list of parameters translatable is
interesting. Didn't know that.
The tests feel bloated to me, bumping the time it takes to run 003 by
30%~40% or so. The "Three conflicting targets" has for example little
value. I think that we should also get rid of most of the tests where
we do the patterns for "-c recovery_target_foo=bar -c
recovery_target_foo=", repeated for each target type. One should be
enough. Keeping only one failure case should be enough for two
recovery targets (or all of them set, why not).
With the first tests in place, I am not convinced that the
anti-pattern "-c recovery_target_foo= -c recovery_target_foo=bar" is
needed. Let's just remove it.
This code is clearly AI-generated. Comments are equally bloated with
descriptions that can be understood just by reading the code. Let's
simplify all that.
In CheckRecoveryTargetConflicts(), initStringInfo() does an
allocation. Perhaps we should free it after making sure we don't
FATAL. Just a good practice.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-25 08:00 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-06-26 07:15 ` Kyotaro Horiguchi <[email protected]>
2026-06-26 08:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Kyotaro Horiguchi @ 2026-06-26 07:15 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hello,
I spent some time thinking about this issue and the proposed patch,
and I wonder if we can simplify it a bit further.
If we don't want to rely on GUC hooks for cross-checking multiple
recovery target settings, perhaps the assign hooks could stop updating
the shared recoveryTarget state altogether and only maintain their own
underlying variables.
As far as I can tell, the existing recovery target variables already
seem sufficient to determine whether each target is configured. The
only exception appears to be recovery_target = 'immediate', which
would probably need an additional boolean flag (or similar state) to
represent whether it has been specified.
Then validateRecoveryParameters() could determine which recovery
target is configured by inspecting those variables directly, report an
error if more than one target is set, and reconstruct the same
recoveryTarget state that the current code expects.
Does that make sense?
Regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-25 08:00 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-26 07:15 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Kyotaro Horiguchi <[email protected]>
@ 2026-06-26 08:12 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2026-06-26 08:12 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Fri, Jun 26, 2026 at 04:15:53PM +0900, Kyotaro Horiguchi wrote:
> As far as I can tell, the existing recovery target variables already
> seem sufficient to determine whether each target is configured. The
> only exception appears to be recovery_target = 'immediate', which
> would probably need an additional boolean flag (or similar state) to
> represent whether it has been specified.
>
> Then validateRecoveryParameters() could determine which recovery
> target is configured by inspecting those variables directly, report an
> error if more than one target is set, and reconstruct the same
> recoveryTarget state that the current code expects.
And delay setting recoveryTarget at all until we enter the validation
step in the startup process? We don't use it in any of the early
steps before entering validateRecoveryParameters() (if we do so, that
would be an incorrect thing to do anyway).
I don't see on top of my mind why that would not work, skipping all
the RECOVERY_TARGET_UNSET manipulations in the assign hooks. One
source of the confusion, to me, is that we have taken the decision to
link directly recoveryTarget with the result of the GUC, while
recoveryTarget is also used to track if the other target GUCs are set.
It looks to me that we shouldn't use recoveryTarget to track both if a
target is set and if the immediate state is set, and split both
things. This is what I guess you are getting at with your extra
boolean. Let's call it a new recoveryTargetImmediate.
At the end recoveryTarget should never be touched in any of the GUC
hooks, just once we enter validateRecoveryParameters().
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-26 11:12 ` Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
1 sibling, 1 reply; 25+ messages in thread
From: Henson Choi @ 2026-06-26 11:12 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
Hi hackers,
Thanks for v6. I think there is still one reachable case on the
command-line (-c) path where the conflict detection is bypassed and a
recovery target is silently dropped, so recovery runs to the end of WAL
instead of stopping at the requested target. On master the same input
is rejected at startup, so for the -c path this is a loud-reject ->
silent behavior change.
Reproduction
------------
Using --check so it is side-effect-free; the conflict is detected during
GUC assignment, so an actual startup behaves the same:
postgres --check -D data \
-c "recovery_target_xid=700" \
-c "recovery_target_name=foo" \
-c "recovery_target_name="
master rejects it at the second assignment:
FATAL: multiple recovery targets specified
DETAIL: At most one of "recovery_target", "recovery_target_lsn",
"recovery_target_name", "recovery_target_time", "recovery_target_xid"
may be set.
v6 passes the check with no error (exit 0); recoveryTarget ends up
RECOVERY_TARGET_UNSET. In an actual PITR this means
recovery_target_xid=700 is silently ignored and the server recovers to
the end of WAL (target overshoot).
postgresql.conf does not reproduce this: duplicate keys are collapsed
there, so only the final value reaches the assign hook. The
set-then-clear of a *second*, different target only happens via -c,
where each option is applied in order and fires the assign hook every
time.
Why it happens
--------------
recoveryTarget is a single enum that the per-GUC assign hooks maintain
incrementally, and it alone drives recovery. CheckRecoveryTargetConflicts()
independently re-reads the committed GUC strings via GetConfigOption().
With the three -c options above the two sides diverge:
step assign hooks -> recoveryTarget final strings
xid=700 XID xid="700"
name=foo NAME (overwrites XID) xid="700", name="foo"
name="" UNSET (NAME was the last type) xid="700", name=""
At "name=foo" two targets are momentarily set, but the single enum
cannot represent that, so it just overwrites; "name=" then clears it.
By the time CheckRecoveryTargetConflicts() runs, the strings have
settled to a single non-empty target (xid), so ntargets == 1 and the
check passes -- while recovery actually runs with recoveryTarget ==
UNSET.
In other words, the v6 matrix row "cross-GUC, both non-empty -> error"
no longer holds once the second GUC is subsequently cleared: the
set-time guard was removed, and the startup check only inspects the
final strings, which no longer show the transient conflict. This is the
same class of unexpected behavior reported earlier in this thread; the
three-step -c form is the variant that still slips through.
One option would be to derive recoveryTarget from the settled GUC strings
in CheckRecoveryTargetConflicts() instead of maintaining it incrementally
in the assign hooks, but any other approach is perfectly fine.
Minor nit
---------
While here: the new ereport() in CheckRecoveryTargetConflicts() still
wraps its arguments in the legacy extra parentheses,
ereport(FATAL,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("multiple recovery targets specified"),
...));
Since the parenthesis-free ereport() form is available, new code can drop
them:
ereport(FATAL,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("multiple recovery targets specified"),
...);
Regards,
Henson
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
@ 2026-06-29 04:45 ` JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-29 04:45 UTC (permalink / raw)
To: [email protected]; +Cc: Álvaro Herrera <[email protected]>; Scott Ray <[email protected]>; Fujii Masao <[email protected]>; Michael Paquier <[email protected]>; [email protected]
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
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-06-29 05:47 ` Michael Paquier <[email protected]>
2026-06-29 07:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
0 siblings, 2 replies; 25+ messages in thread
From: Michael Paquier @ 2026-06-29 05:47 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: [email protected]; Álvaro Herrera <[email protected]>; Scott Ray <[email protected]>; Fujii Masao <[email protected]>; [email protected]
On Mon, Jun 29, 2026 at 01:45:37PM +0900, JoongHyuk Shin wrote:
> 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.
Hmm. One aspect of the patch that I have a hard time accepting is
this aspect:
ADD_TARGET_IF_SET("recovery_target", RECOVERY_TARGET_IMMEDIATE);
This part lacks future extensibility, IMO. If we add more values to
the GUC recovery_target in the future, we push down more complication
to the resolution of the immediate target at the beginning of the
startup process. Decoupling entirely RecoveryTargetType and the types
of values that can be assigned in the GUC recovery_target may lead to
a nicer result, I suspect.. It also feels wasteful to add an
hypothetical enum for the supported values if we don't have a use for
it yet. Any opinions from others?
+ 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."));
pg_settings is just one way to look at these values. We have also
SHOW and other interfaces. I would keep the errdetail() with your
first sentence, make the errhint the second sentence of the
errdetail().
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-06-29 07:59 ` JoongHyuk Shin <[email protected]>
1 sibling, 0 replies; 25+ messages in thread
From: JoongHyuk Shin @ 2026-06-29 07:59 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]; Álvaro Herrera <[email protected]>; Scott Ray <[email protected]>; Fujii Masao <[email protected]>; [email protected]
On Mon, Jun 29, 2026 at 2:48 PM Michael Paquier <[email protected]> wrote:
> pg_settings is just one way to look at these values. We have also
> SHOW and other interfaces. I would keep the errdetail() with your
> first sentence, make the errhint the second sentence of the errdetail().
I split it along the message style guide's line, factual information
in the detail and suggestions in the hint. The detail states the facts,
which targets are set, and the "see pg_settings" pointer is guidance
for tracking them down rather than a fact about the conflict,
so it went into the hint.
I'd keep it there rather than fold it into the detail.
SHOW does return the values, but I named pg_settings
specifically because it also reports where each parameter is set,
through its sourcefile and sourceline columns.
With several recovery_target* values possibly spread
across configuration files, finding where each one is set
is usually what it takes to resolve the conflict,
which is the part SHOW leaves out.
--
JH Shin
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-06-29 21:16 ` Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
1 sibling, 1 reply; 25+ messages in thread
From: Zsolt Parragi @ 2026-06-29 21:16 UTC (permalink / raw)
To: [email protected]
On Mon, 29 Jun 2026, Michael Paquier <[email protected]> wrote:
> + 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."));
>
> pg_settings is just one way to look at these values. We have also
> SHOW and other interfaces. I would keep the errdetail() with your
> first sentence, make the errhint the second sentence of the
> errdetail().
Isn't mentioning pg_settings confusing instead of helpful during a
server restart? With a reload it can help, but when the server can't
start, hinting that the user should query pg_settings doesn't seem
that useful.
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
@ 2026-06-29 23:35 ` Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2026-06-29 23:35 UTC (permalink / raw)
To: Zsolt Parragi <[email protected]>; +Cc: [email protected]
On Mon, Jun 29, 2026 at 04:16:59PM -0500, Zsolt Parragi wrote:
> Isn't mentioning pg_settings confusing instead of helpful during a
> server restart? With a reload it can help, but when the server can't
> start, hinting that the user should query pg_settings doesn't seem
> that useful.
Yeah, it is. I don't see a reason why we should be specific about the
location where these parameters are set. One has been setting up
recovery parameters in the GUC machine, so they most likely know where
these strings are.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-07-06 07:53 ` JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: JoongHyuk Shin @ 2026-07-06 07:53 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; [email protected]
On Mon, Jun 29, 2026 at 04:16:59PM -0500, Zsolt Parragi wrote:
> Isn't mentioning pg_settings confusing instead of helpful during a
> server restart? With a reload it can help, but when the server can't
> start, hinting that the user should query pg_settings doesn't seem
> that useful.
You're right, and I had missed this. Thanks.
I would rather drop the errhint from this patch entirely than fold it into
the errdetail.
Any objections?
--
JH Shin
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-07-06 22:30 ` Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2026-07-06 22:30 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; [email protected]
On Mon, Jul 06, 2026 at 04:53:44PM +0900, JoongHyuk Shin wrote:
> I would rather drop the errhint from this patch entirely than fold it into
> the errdetail.
> Any objections?
Sounds good here to remove the errhint(). No hint is better than a
potentially wrong hint. Thanks.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-07-07 03:01 ` JoongHyuk Shin <[email protected]>
2026-07-07 06:58 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 06:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
0 siblings, 2 replies; 25+ messages in thread
From: JoongHyuk Shin @ 2026-07-07 03:01 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; [email protected]
Attached v8 with the errhint removed. No other changes from v7.
--
JH Shin
Attachments:
[application/octet-stream] v8-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch (17.4K, ../../CACSdjfPm+_EE2BTmu7oyW_Gt_7o2eZyNdvaT3fQKbaoKCwGOMQ@mail.gmail.com/3-v8-0001-Don-t-call-ereport-ERROR-from-recovery-target-GUC.patch)
download | inline diff:
From decdc03a2177ed31b5bbb464fa8c67d3f6d922bf Mon Sep 17 00:00:00 2001
From: JoongHyuk Shin <[email protected]>
Date: Sun, 28 Jun 2026 18:25:45 +0900
Subject: [PATCH v8] 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 | 141 ++++++++-----------
src/backend/utils/misc/guc_parameters.dat | 2 -
src/include/utils/guc_hooks.h | 2 -
src/test/recovery/t/003_recovery_targets.pl | 145 ++++++++++++++++----
4 files changed, 178 insertions(+), 112 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c0ae4d3f63f..e0c9d306e6d 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,58 @@ 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));
+
+ pfree(buf.data);
+
+ return target;
}
/*
@@ -4809,22 +4847,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 +4878,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 +4904,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 +4969,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 +5087,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..19b25368089 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,65 @@ 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');
+
+# 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
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-07-07 06:58 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2026-07-07 06:58 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; [email protected]
On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
> Attached v8 with the errhint removed. No other changes from v7.
+ errmsg("multiple recovery targets specified"),
+ errdetail("Only one recovery target can be set. Parameters set: %s.",
+ buf.data));
FWIW, I think that this is redundant, as the errmsg and the errdetail
are basically saying the same thing. I'd suggest a simpler:
errmsg: cannot specify more than one recovery target
errdetail: Parameters set are: %s.
+# that does not clear another GUC's target. Conflicts are detected at every
+# server start by DetermineRecoveryTargetType().
We don't really care about the function name here, just that multiple
targets are blocked.
+# 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.
I smell of an AI set of comments. We could just remove the whole and
not lose value in understanding the meaning of the test. A bunch of
the comments added to the TAP script could also be trimmed down quite
a bit, made simpler..
With recovery_target assign hook being removed for the case of
immediate, a test case that checks for a conflict between immediate
and a secondary target may be in order.
Please note that Fujii-san is registered as a committer of this patch,
so I am not planning to go beyond a review here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
@ 2026-07-07 06:59 ` Michael Paquier <[email protected]>
2026-07-08 01:38 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Fujii Masao <[email protected]>
1 sibling, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2026-07-07 06:59 UTC (permalink / raw)
To: JoongHyuk Shin <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; [email protected]
On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
> Attached v8 with the errhint removed. No other changes from v7.
+ errmsg("multiple recovery targets specified"),
+ errdetail("Only one recovery target can be set. Parameters set: %s.",
+ buf.data));
FWIW, I think that this is redundant, as the errmsg and the errdetail
are basically saying the same thing. I'd suggest a simpler:
errmsg: cannot specify more than one recovery target
errdetail: Parameters set are: %s.
+# that does not clear another GUC's target. Conflicts are detected at every
+# server start by DetermineRecoveryTargetType().
We don't really care about the function name here, just that multiple
targets are blocked.
+# 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.
I smell of an AI set of comments. We could just remove the whole and
not lose value in understanding the meaning of the test. A bunch of
the comments added to the TAP script could also be trimmed down quite
a bit, made simpler..
With recovery_target assign hook being removed for the case of
immediate, a test case that checks for a conflict between immediate
and a secondary target may be in order.
Please note that Fujii-san is registered as a committer of this patch,
so I am not planning to go beyond a review here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-07 06:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
@ 2026-07-08 01:38 ` Fujii Masao <[email protected]>
2026-07-08 01:43 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
0 siblings, 1 reply; 25+ messages in thread
From: Fujii Masao @ 2026-07-08 01:38 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: JoongHyuk Shin <[email protected]>; Zsolt Parragi <[email protected]>; [email protected]
On Tue, Jul 7, 2026 at 3:59 PM Michael Paquier <[email protected]> wrote:
>
> On Tue, Jul 07, 2026 at 12:01:59PM +0900, JoongHyuk Shin wrote:
> > Attached v8 with the errhint removed. No other changes from v7.
We can remove assign_recovery_target_name()? With the v8 patch, it only
assigns the GUC string pointer to recoveryTargetName. It seems we could
instead have recovery_target_name store its value directly in
recoveryTargetName, remove recovery_target_name_string, and drop
the assign hook altogether. Thoughts?
> Please note that Fujii-san is registered as a committer of this patch,
> so I am not planning to go beyond a review here.
Thanks for the review! Yes, I will handle this patch.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-07 06:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-08 01:38 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Fujii Masao <[email protected]>
@ 2026-07-08 01:43 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2026-07-08 01:43 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: JoongHyuk Shin <[email protected]>; Zsolt Parragi <[email protected]>; [email protected]
On Wed, Jul 08, 2026 at 10:38:54AM +0900, Fujii Masao wrote:
> We can remove assign_recovery_target_name()? With the v8 patch, it only
> assigns the GUC string pointer to recoveryTargetName. It seems we could
> instead have recovery_target_name store its value directly in
> recoveryTargetName, remove recovery_target_name_string, and drop
> the assign hook altogether. Thoughts?
Yeah, perhaps we should just do that. I've also found the consistency
with all these _string variables interesting to keep, but that's
minor compared to less code.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2026-07-08 01:43 UTC | newest]
Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v45 1/7] sequential scan for dshash Kyotaro Horiguchi <[email protected]>
2026-05-31 21:11 Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-04 05:41 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-06 20:10 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Scott Ray <[email protected]>
2026-06-07 10:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-07 15:44 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-21 09:27 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-21 12:31 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Álvaro Herrera <[email protected]>
2026-06-22 04:37 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-25 08:00 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-26 07:15 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Kyotaro Horiguchi <[email protected]>
2026-06-26 08:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-26 11:12 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Henson Choi <[email protected]>
2026-06-29 04:45 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 05:47 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-06-29 07:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-06-29 21:16 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Zsolt Parragi <[email protected]>
2026-06-29 23:35 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-06 07:53 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-06 22:30 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 03:01 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks JoongHyuk Shin <[email protected]>
2026-07-07 06:58 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-07 06:59 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
2026-07-08 01:38 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Fujii Masao <[email protected]>
2026-07-08 01:43 ` Re: [PATCH] Don't call ereport(ERROR) from recovery target GUC assign hooks Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox