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