From: David Steele <david@pgbackrest.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve verification of recovery_target_timeline GUC.
Date: Fri, 25 Apr 2025 13:50:16 +0000 (UTC)
Message-ID: <08cf439a-4566-4698-b60f-d8e9ed348456@pgbackrest.org> (raw)
In-Reply-To: <aArTehpV_Mr8Lqf_@paquier.xyz>
References: <e5d472c7-e9be-4710-8dc4-ebe721b62cea@pgbackrest.org>
<Z5M2tgQUMgCkyrKJ@paquier.xyz>
<a17d1bf9-8123-4b86-9c84-5b7526cd70fd@pgbackrest.org>
<Z67z1UndsbKGOuLt@paquier.xyz>
<ac0d1e55-75f6-40d1-882d-05274811f294@pgbackrest.org>
<aArTehpV_Mr8Lqf_@paquier.xyz>
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
Attachments:timeline-check-v3.patch (text/plain, 3.8K) download
From 5e8fa391e7d2171475dc8c86ef29fe3c9305d2e0 Mon Sep 17 00:00:00 2001
From: David Steele <david@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.cindex 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.plindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: david@pgbackrest.org, michael@paquier.xyz
Subject: Re: Improve verification of recovery_target_timeline GUC.
In-Reply-To: <08cf439a-4566-4698-b60f-d8e9ed348456@pgbackrest.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox