On 2/14/25 02:42, Michael Paquier wrote:
On Fri, Jan 24, 2025 at 01:36:45PM +0000, David Steele wrote:
+ timeline = strtoull(*newval, &endp, 0);
+
+ if (*endp != '\0' || errno == EINVAL || errno == ERANGE)
{
GUC_check_errdetail("\"recovery_target_timeline\" is not a
valid number.");
return false;
}
Why not using strtou64() here? That's more portable depending on
SIZEOF_LONG (aka the 4 bytes on WIN32 vs 8 bytes for the rest of the
world).
Done.
+
+ if (timeline < 2 || timeline > UINT_MAX)
+ {
A recovery_target_timeline of 1 is a valid value, isn't it?
+ GUC_check_errdetail(
+ "\"recovery_target_timeline\" must be between 2 and
4,294,967,295.");
+ return false;
+ }
I would suggest to rewrite that as \"%s\" must be between %u and %u,
which would be more generic for translations in case there is an
overlap with something else.
Done. This means there are no commas in the upper bound but I don't
think it's a big deal and it more closely matches other GUC messages.
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/must be between 2 and 4,294,967,295/,
+ 'target timelime out of max range');
These sequences of tests could be improved:
- Let's use start locations for the logs slurped, reducing the scope
of the logs scanned.
Done.
- Perhaps it would be better to rely on wait_for_log() to make sure
that the expected log contents are generated without any kind of race
condition?
Done.
I'm also now using a single cluster for all three tests rather than
creating a new one for each test. This is definitely more efficient.
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.
Thanks,
-David
From 2ccf6f207a14dafe1a426208618ae7690c4513a2 Mon Sep 17 00:00:00 2001
From: David Steele <da...@pgmasters.net>
Date: Thu, 24 Apr 2025 15:11:51 +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..bd701d04f99 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 recovery startup fails');
+
+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 recovery startup fails');
+
+$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 recovery startup fails');
+
+$log_start = $node_standby->wait_for_log("must be between 1 and 4294967295",
+
$log_start);
+
done_testing();
--
2.34.1
