From c61284c824429778ad8f123e22862931d41a2a6d Mon Sep 17 00:00:00 2001 From: Scott Ray 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)