On 3/4/26 22:41, Fujii Masao wrote:
Regarding the regression test, if the purpose is to verify the GUC hook
for recovery_target_xid, it might be simpler to test whether
"ALTER SYSTEM SET recovery_target_xid TO ..." succeeds or fails as expected
as follows, rather than starting the server with that setting. That said,
since recovery_target_timeline is already tested in a similar way, I understand
why you followed the same pattern here. So I'm ok with the current approach.
I wrote the tests for recovery_target_timeline but I was not too
satisfied with them because starting Postgres is fairly expensive.
my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
"ALTER SYSTEM SET recovery_target_xid TO 'bogus'");
like(
$stderr,
qr/is not a valid number/,
"invalid recovery_target_xid (bogus value)");
If we think it's better to use ALTER SYSTEM SET for testing invalid
recovery_target_xxx settings to keep the regression tests simpler,
we can revisit this later and address it in a separate patch.
I've updated the patch to do it this way. Not only is it faster but you
get a better message when the expected value is incorrect.
I can update the tests for recovery_target_timeline in a separate patch.
Regards,
-David
From b2be59a52a7a74cd0a6c83619fa782a92b7ab9a0 Mon Sep 17 00:00:00 2001
From: David Steele <[email protected]>
Date: Thu, 5 Mar 2026 03:35:32 +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.
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).
Also update the documentation for recovery_target_xid to clarify usage.
---
doc/src/sgml/config.sgml | 15 ++++++++++
src/backend/access/transam/xlogrecovery.c | 31 +++++++++++++++++++--
src/test/recovery/t/003_recovery_targets.pl | 27 ++++++++++++++++++
3 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f670e2d4c31..949b86e4e70 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4334,6 +4334,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f"
"%p"' # Windows
The precise stopping point is also influenced by
<xref linkend="guc-recovery-target-inclusive"/>.
</para>
+
+ <para>
+ The value can be specified as either a 32-bit transaction ID or a
64-bit
+ transaction ID (consisting of an epoch and a 32-bit ID), such as the
+ value returned by <function>pg_current_xact_id()</function>. When a
+ 64-bit transaction ID is provided, only its 32-bit transaction ID
+ portion is used as the recovery target. For example, the values
+ 4294968296 (epoch 1) and 8589935592 (epoch 2) both refer to the same
+ 32-bit transaction ID, 1000.
+ </para>
+
+ <para>
+ The effective transaction ID (the 32-bit portion) must be greater than
+ or equal to 3.
+ </para>
</listitem>
</varlistentry>
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..f5da13128d6 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -240,4 +240,31 @@ 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 tests
+$node_standby = PostgreSQL::Test::Cluster->new('standby_10');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->start;
+
+my ($result, $stdout, $stderr) = $node_primary->psql('postgres',
+ "ALTER SYSTEM SET recovery_target_xid TO 'bogus'");
+like(
+ $stderr,
+ qr/is not a valid number/,
+ "invalid recovery_target_xid (bogus value)");
+
+($result, $stdout, $stderr) = $node_primary->psql('postgres',
+ "ALTER SYSTEM SET recovery_target_xid TO '-1'");
+like(
+ $stderr,
+ qr/is not a valid number/,
+ "invalid recovery_target_xid (negative)");
+
+($result, $stdout, $stderr) = $node_primary->psql('postgres',
+ "ALTER SYSTEM SET recovery_target_xid TO '0'");
+like(
+ $stderr,
+ qr/without epoch must be greater than or equal to 3/,
+ "invalid recovery_target_xid (lower bound check)");
+
done_testing();
--
2.34.1
