On 4/24/25 20:12, Michael Paquier wrote:
On Thu, Apr 24, 2025 at 03:34:29PM +0000, David Steele wrote:
Having said that, I think these tests are awfully expensive for a single
GUC. Unit tests would definitely be preferable but that's not an option for
GUCs, so far as I know.
On my laptop, 003_recovery_targets.pl increases from 8.2s to 8.7s,
which seems OK here for the coverage we have.
Multiply half a second by similar tests on all the GUCs and it would add
up to a lot. But no argument here on keeping the tests.
"invalid recovery startup fails" is used three times repeatedly for
the tests with bogus and out-of-bound values. I'd suggest to make
these more verbose, with three extras "bogus value", "lower bound
check" and "upper bound check" added.
I have updated the labels to be more descriptive.
Side thing noted while reading the area: check_recovery_target_xid()
does not have any GUC_check_errdetail() giving details for the EINVAL
and ERANGE cases. Your addition for recovery_target_timeline is a
good idea, so perhaps we could do the same there? No need to do that,
that's just something I've noticed in passing..
I don't want to add that to this commit but I have added a note to my
list of PG improvements to make when I have time.
And you are following the fat-comma convention for the command lines,
thanks for doing that.
Just trying to follow the convention from existing tests, but you are
welcome!
Note that I'm not planning to do anything here
for v18 now that we are in feature freeze, these would be for v19 once
the branch opens.
That was my expectation. I just had some time to get this patch updated
so took the opportunity.
Regards,
-David
From 5e8fa391e7d2171475dc8c86ef29fe3c9305d2e0 Mon Sep 17 00:00:00 2001
From: David Steele <da...@pgmasters.net>
Date: Fri, 25 Apr 2025 13:37:41 +0000
Subject: Improve verification of recovery_target_timeline GUC.
Currently check_recovery_target_timeline() converts any value that is not
current, latest, or a valid integer to 0. So for example:
recovery_target_timeline = 'currrent'
results in the following error:
FATAL: 22023: recovery target timeline 0 does not exist
Since there is no range checking for uint32 (but there is a cast from unsigned
long) this setting:
recovery_target_timeline = '9999999999'
results in the following error:
FATAL: 22023: recovery target timeline 1410065407 does not exist
Improve by adding endptr checking to catch conversion errors and add range
checking to
exclude values < 2 and greater than UINT_MAX.
---
src/backend/access/transam/xlogrecovery.c | 17 +++++--
src/test/recovery/t/003_recovery_targets.pl | 50 +++++++++++++++++++++
2 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index 6ce979f2d8b..e82aa739d79 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -4994,13 +4994,24 @@ check_recovery_target_timeline(char **newval, void
**extra, GucSource source)
rttg = RECOVERY_TARGET_TIMELINE_LATEST;
else
{
+ char *endp;
+ uint64 timeline;
rttg = RECOVERY_TARGET_TIMELINE_NUMERIC;
errno = 0;
- strtoul(*newval, NULL, 0);
- if (errno == EINVAL || errno == ERANGE)
+ timeline = strtou64(*newval, &endp, 0);
+
+ if (*endp != '\0' || errno == EINVAL || errno == ERANGE)
+ {
+ GUC_check_errdetail("\"%s\" is not a valid number.",
+
"recovery_target_timeline");
+ return false;
+ }
+
+ if (timeline < 1 || timeline > UINT_MAX)
{
- GUC_check_errdetail("\"recovery_target_timeline\" is
not a valid number.");
+ GUC_check_errdetail("\"%s\" must be between %u and %u.",
+
"recovery_target_timeline", 1, UINT_MAX);
return false;
}
}
diff --git a/src/test/recovery/t/003_recovery_targets.pl
b/src/test/recovery/t/003_recovery_targets.pl
index 0ae2e982727..a963f46b7bb 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -187,4 +187,54 @@ ok( $logfile =~
qr/FATAL: .* recovery ended before configured recovery target was
reached/,
'recovery end before target reached is a fatal error');
+# Invalid timeline target
+$node_standby = PostgreSQL::Test::Cluster->new('standby_9');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = 'bogus'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid timeline target (bogus value)');
+
+my $log_start = $node_standby->wait_for_log("is not a valid number");
+
+# Timeline target out of min range
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = '0'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid timeline target (lower bound check)');
+
+$log_start = $node_standby->wait_for_log("must be between 1 and 4294967295",
+
$log_start);
+
+# Timeline target out of max range
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = '4294967296'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid timeline target (upper bound check)');
+
+$log_start = $node_standby->wait_for_log("must be between 1 and 4294967295",
+
$log_start);
+
done_testing();
--
2.34.1
