]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Avoid ERROR in recovery target GUC assign hooks
authorFujii Masao <fujii@postgresql.org>
Mon, 20 Jul 2026 12:41:14 +0000 (21:41 +0900)
committerFujii Masao <fujii@postgresql.org>
Mon, 20 Jul 2026 12:42:19 +0000 (21:42 +0900)
Recovery target parameters are postmaster-startup GUCs, but their
assign hooks previously did more than assign individual parameter
values. They also updated the global recoveryTarget state and raised
ERROR if more than one recovery target appeared to be set.

This was not a good fit for GUC assign hooks. Assign hooks should not
throw ERROR, and deriving cross-parameter state while individual GUCs
are still being assigned makes the result depend on assignment order
rather than the final configuration.

For example, setting one recovery target and then setting another
recovery_target_* parameter to an empty string could clear
recoveryTarget, causing recovery to proceed with no target even
though a valid target remained configured.

Fix this by having the assign hooks only store their own parameter
values. The effective recoveryTarget is now derived once from the
final recovery_target* settings in
validateRecoveryParameters(), which also rejects configurations that
specify more than one recovery target with FATAL. This preserves the
expected behavior for repeated assignments of the same GUC, treats empty
values as "not set", and removes cross-GUC validation from the assign
hooks.

Author: JoongHyuk Shin <sjh910805@gmail.com>
Reviewed-by: Greg Lamberson <greg@lamco.io>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Scott Ray <scott@scottray.io>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Henson Choi <assam258@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CACSdjfPUa4UvKjADgOERXoxNYmCg2mqqiqKkiJk6mX6E4qgVFw@mail.gmail.com

src/backend/access/transam/xlogrecovery.c
src/backend/utils/misc/guc_parameters.dat
src/backend/utils/misc/guc_tables.c
src/include/access/xlogrecovery.h
src/include/utils/guc_hooks.h
src/test/recovery/t/003_recovery_targets.pl

index a9ebac2d0ef31688591e771a690204e7a9b2723f..5f3b065b8942e20ed53a86b0489bb1153168cc87 100644 (file)
@@ -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"
@@ -92,7 +93,7 @@ int                   recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
 TransactionId recoveryTargetXid;
 char      *recovery_target_time_string;
 TimestampTz recoveryTargetTime;
-const char *recoveryTargetName;
+char      *recoveryTargetName;
 XLogRecPtr     recoveryTargetLSN;
 int                    recovery_min_apply_delay = 0;
 
@@ -392,6 +393,7 @@ static bool HotStandbyActiveInReplay(void);
 
 static void SetCurrentChunkStartTime(TimestampTz xtime);
 static void SetLatestXTime(TimestampTz xtime);
+static RecoveryTargetType DetermineRecoveryTargetType(void);
 
 /*
  * Register shared memory for WAL recovery
@@ -1067,6 +1069,9 @@ readRecoverySignalFile(void)
 static void
 validateRecoveryParameters(void)
 {
+       /* Reject conflicting targets even when recovery was not requested */
+       recoveryTarget = DetermineRecoveryTargetType();
+
        if (!ArchiveRecoveryRequested)
                return;
 
@@ -4769,30 +4774,50 @@ 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.
+ * Return the recovery target derived from the recovery_target* settings,
+ * raising an error if more than one of them is set.
  */
-
-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);
+
+#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("cannot specify more than one recovery target"),
+                               errdetail("Parameters set are: %s.",
+                                                 buf.data));
+
+       pfree(buf.data);
+
+       return target;
 }
 
 /*
@@ -4809,22 +4834,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 +4865,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;
 }
 
 /*
@@ -4885,25 +4885,6 @@ check_recovery_target_name(char **newval, void **extra, GucSource source)
        return true;
 }
 
-/*
- * GUC assign_hook for recovery_target_name
- */
-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;
-}
-
 /*
  * GUC check_hook for recovery_target_time
  *
@@ -4965,22 +4946,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 +5064,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;
 }
index d421cdbde76daf628777c6721ea644949039591e..adb72361ce0bb64c49e5cf53c9bc47c747f63da1 100644 (file)
   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',
 
 { name => 'recovery_target_name', type => 'string', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET',
   short_desc => 'Sets the named restore point up to which recovery will proceed.',
-  variable => 'recovery_target_name_string',
+  variable => 'recoveryTargetName',
   boot_val => '""',
   check_hook => 'check_recovery_target_name',
-  assign_hook => 'assign_recovery_target_name',
 },
 
 { name => 'recovery_target_time', type => 'string', context => 'PGC_POSTMASTER', group => 'WAL_RECOVERY_TARGET',
   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',
index 90aa374b3ec70836a18d03f19c17ae735541e486..1ec460b6a8236ef160eb28d63244d0e7bc264453 100644 (file)
@@ -667,7 +667,6 @@ static bool exec_backend_enabled = EXEC_BACKEND_ENABLED;
 static char *recovery_target_timeline_string;
 static char *recovery_target_string;
 static char *recovery_target_xid_string;
-static char *recovery_target_name_string;
 static char *recovery_target_lsn_string;
 
 /* should be static, but commands/variable.c needs to get at this */
index ba7750dca0b453255dcec212a41e3dc6d1df799c..9ffd44fcbaebf16a121da328bb523680395b3cfc 100644 (file)
@@ -139,7 +139,7 @@ extern PGDLLIMPORT char *archiveCleanupCommand;
 extern PGDLLIMPORT TransactionId recoveryTargetXid;
 extern PGDLLIMPORT char *recovery_target_time_string;
 extern PGDLLIMPORT TimestampTz recoveryTargetTime;
-extern PGDLLIMPORT const char *recoveryTargetName;
+extern PGDLLIMPORT char *recoveryTargetName;
 extern PGDLLIMPORT XLogRecPtr recoveryTargetLSN;
 extern PGDLLIMPORT RecoveryTargetType recoveryTarget;
 extern PGDLLIMPORT bool wal_receiver_create_temp_slot;
index 307f4fbaefe08f4ac1f2441dab29664d90501f14..6a76f8d5ed6cc6b7c014d3ed9c64afc855576c16 100644 (file)
@@ -103,16 +103,13 @@ 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);
 extern bool check_recovery_target_name(char **newval, void **extra,
                                                                           GucSource source);
-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);
index 047eb13293ae39541cb75fc76976308167ac8638..0469afb5bde5459d7cd3c920238d12e07e201102 100644 (file)
@@ -12,6 +12,10 @@ use Time::HiRes qw(usleep);
 # Create and test a standby from given backup, with a certain recovery target.
 # Choose $until_lsn later than the transaction commit that causes the row
 # count to reach $num_rows, yet not later than the recovery target.
+# If options is given, pass it through Cluster->start(options => ...).  This is
+# 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
 {
        local $Test::Builder::Level = $Test::Builder::Level + 1;
@@ -22,6 +26,7 @@ sub test_recovery_standby
        my $recovery_params = shift;
        my $num_rows = shift;
        my $until_lsn = shift;
+       my %params = @_;
 
        my $node_standby = PostgreSQL::Test::Cluster->new($node_name);
        $node_standby->init_from_backup($node_primary, 'my_backup',
@@ -32,7 +37,8 @@ sub test_recovery_standby
                $node_standby->append_conf('postgresql.conf', qq($param_item));
        }
 
-       $node_standby->start;
+       $node_standby->start(
+               defined $params{options} ? (options => $params{options}) : ());
 
        # Wait until standby has replayed enough data
        my $caughtup_query =
@@ -108,6 +114,9 @@ $node_primary->safe_psql('postgres',
 # Force archiving of WAL file
 $node_primary->safe_psql('postgres', "SELECT pg_switch_wal()");
 
+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 +134,19 @@ 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, but setting the same
+# parameter twice or clearing one with an empty string is allowed.
 
 @recovery_params = (
        "recovery_target_name = '$recovery_name'",
@@ -138,31 +155,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 +179,84 @@ 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'");
+
+ok( !$node_no_signal->start(fail_ok => 1),
+       '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/cannot specify more than one recovery target/,
+       'expected error message logged without recovery.signal');
+like(
+       $logfile_no_signal,
+       qr/Parameters set are: "recovery_target_name", "recovery_target_time"/,
+       'errdetail lists the set parameters in order without recovery.signal');
+unlike(
+       $logfile_no_signal,
+       qr/Parameters set are:[^\n]*=/,
+       'errdetail does not echo parameter values without recovery.signal');
+
+my $node_immediate_conflict =
+  PostgreSQL::Test::Cluster->new('immediate_target_conflict');
+$node_immediate_conflict->init_from_backup($node_primary, 'my_backup');
+$node_immediate_conflict->append_conf(
+       'postgresql.conf',
+       "recovery_target = 'immediate'
+recovery_target_xid = '$recovery_txid'");
+
+ok( !$node_immediate_conflict->start(fail_ok => 1),
+       'server fails to start with recovery_target=immediate and a second target'
+);
+like(
+       slurp_file($node_immediate_conflict->logfile()),
+       qr/cannot specify more than one recovery target/,
+       'recovery_target=immediate conflicting with another target is rejected');
+
+# 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(
+       'recovery_target_xid set then cleared',
+       'standby_xid_set_clear',
+       $node_primary,
+       [],
+       "6000",
+       $lsn6,
+       options => "-c recovery_target_xid=$recovery_txid -c recovery_target_xid="
+);
+
+# 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(
+       'recovery target preserved when a competing one is set then cleared',
+       'standby_clobber_clear',
+       $node_primary,
+       [],
+       "2000",
+       $lsn2,
+       options =>
+         "-c recovery_target_xid=$recovery_txid -c recovery_target_name=$recovery_name -c recovery_target_name="
+);
+
 # Invalid recovery_target_timeline tests
 my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
        "ALTER SYSTEM SET recovery_target_timeline TO 'bogus'");