Thanks for having a look at this!
On 2/26/26 14:20, Hüseyin Demir wrote:
The following grammar can be changed by adding "without epoch must be greater than
or equal to %u"
+ GUC_check_errdetail("\"%s\" without epoch must greater than or equal to
%u.",
+ "recovery_target_xid",
+ FirstNormalTransactionId);
Oops - fixed!
The comment on the lower-bound XID test says # Timeline target out of min range
— should be # XID target out of min range.
I have fixed this and made the comments more consistent overall.
When it comes to *endp validations I suppose the validation passes when we
provide recovery_target_xid = '-1'. This passes the endp validation and
FirstNormalTransactionId checks. Is it a valid approach to allow negative
values to this GUC ?
When -1 is provided the following checks allow them to be a valid GUC.
Yeah, -1 should not be allowed here. I've updated the code to error on
negative numbers but probably we should import strtou64_strict from the
front end code or use strtou32_strict, though that needs to be discussed
separately.
Thanks,
-David
From 488260c30dd65aae2f0b9077641dbe7e195ea8f4 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Wed, 4 Mar 2026 04:47:23 +0000
Subject: Improve checks for GUC recovery_target_xid
Currently check_recovery_target_xid() converts invalid values to 0. So,
for example, the following configuration added to postgresql.conf
followed by a startup:
recovery_target_xid = 'bogus'
recovery_target_xid = '1.1'
recovery_target_xid = '0'
... does not generate an error but recovery does not complete. There are
many values that can prevent recovery from completing but we should at
least catch obvious misconfiguration by the user.
The origin of the problem is that we do not perform a range check in the
GUC value passed-in for recovery_target_xid. This commit improves the
situation by using adding end checking to strtou64() and by providing
stricter range checks. Some test cases are added for the cases of an
incorrect or a lower-bound timeline value, checking the sanity of the
reports based on the contents of the server logs.
Also add a comment that truncation of the input value is expected since
users will generally be using the output from pg_current_xact_id() (or
similar) to set recovery_target_xid (just as our tests do).
---
src/backend/access/transam/xlogrecovery.c | 31 +++++++++++++++++--
src/test/recovery/t/003_recovery_targets.pl | 34 +++++++++++++++++++++
2 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index ecd66fd86a4..fd0345a65f8 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -5108,11 +5108,38 @@ check_recovery_target_xid(char **newval, void **extra,
GucSource source)
{
TransactionId xid;
TransactionId *myextra;
+ char *endp;
+ char *val;
errno = 0;
- xid = (TransactionId) strtou64(*newval, NULL, 0);
- if (errno == EINVAL || errno == ERANGE)
+
+ /*
+ * Consume leading whitespace to determine if number is negative
+ */
+ val = *newval;
+
+ while (isspace((unsigned char)*val))
+ val++;
+
+ /*
+ * This cast will remove the epoch, if any
+ */
+ xid = (TransactionId) strtou64(val, &endp, 0);
+
+ if (*endp != '\0' || errno == EINVAL || errno == ERANGE || *val
== '-')
+ {
+ GUC_check_errdetail("\"%s\" is not a valid number.",
+
"recovery_target_xid");
return false;
+ }
+
+ if (xid < FirstNormalTransactionId)
+ {
+ GUC_check_errdetail("\"%s\" without epoch must be
greater than or equal to %u.",
+
"recovery_target_xid",
+
FirstNormalTransactionId);
+ return false;
+ }
myextra = (TransactionId *) guc_malloc(LOG,
sizeof(TransactionId));
if (!myextra)
diff --git a/src/test/recovery/t/003_recovery_targets.pl
b/src/test/recovery/t/003_recovery_targets.pl
index e0df1a23423..0813394d70a 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -240,4 +240,38 @@ ok(!$res, 'invalid timeline target (upper bound check)');
$log_start =
$node_standby->wait_for_log("must be between 1 and 4294967295", $log_start);
+# Invalid recovery_target_xid (bogus value)
+$node_standby = PostgreSQL::Test::Cluster->new('standby_10');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->append_conf('postgresql.conf',
+ "recovery_target_xid = 'bogus'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery_target_xid (bogus value)');
+
+$log_start = $node_standby->wait_for_log("is not a valid number");
+
+# Invalid recovery_target_xid (lower bound check)
+$node_standby->append_conf('postgresql.conf',
+ "recovery_target_xid = '0'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery_target_xid (lower bound check)');
+
+$log_start = $node_standby->wait_for_log(
+ "without epoch must be greater than or equal to 3", $log_start);
+
done_testing();
--
2.34.1
