pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedImprove verification of recovery_target_timeline GUC.
10+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2026-02-20 0dc22fff64 Fix constant in error message for recovery_target_timeline
2025-07-03 fd7d7b7191 Improve checks for GUC recovery_target_timeline
* Improve verification of recovery_target_timeline GUC.
@ 2025-01-23 14:53 David Steele <david@pgbackrest.org>
0 siblings, 1 reply; 10+ messages in thread
From: David Steele @ 2025-01-23 14:53 UTC (permalink / raw)
To: pgsql-hackers
Hackers,
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 (on 64-bit):
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.
I discovered this while testing on Postgres versions < 12 where
recovery_target_timeline = 'current'
sets the target timeline to 'latest', which is a pretty surprising
result. I'm only noting it in case somebody is searching the archives
since the versions in question are EOL. But that odd result made me
wonder if Postgres >= 12 had a similar problem. While it is not quite so
confusing as Postgres < 12 I think it still bears improving.
The tests are probably excessive but I needed something to show that the
verification works as expected.
Regards,
-David
Attachments:
timeline-log-v1.patch (text/plain, 1.6K) download
From f14e30b18cde216131bd3e069ee8ecd5da3301b0 Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Fri, 20 Dec 2024 15:13:59 +0000
Subject: Fix logging for invalid recovery timeline.
If the requested recovery timeline is not reachable the logged checkpoint and
TLI should to be the values read from backup_label when backup_label is
present.
---
src/backend/access/transam/xlogrecovery.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c6994b78282..99afd01bf38 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -844,13 +844,13 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
* tliSwitchPoint will throw an error if the checkpoint's timeline is
* not in expectedTLEs at all.
*/
- switchpoint = tliSwitchPoint(ControlFile->checkPointCopy.ThisTimeLineID, expectedTLEs, NULL);
+ switchpoint = tliSwitchPoint(CheckPointTLI, expectedTLEs, NULL);
ereport(FATAL,
(errmsg("requested timeline %u is not a child of this server's history",
recoveryTargetTLI),
errdetail("Latest checkpoint is at %X/%X on timeline %u, but in the history of the requested timeline, the server forked off from that timeline at %X/%X.",
- LSN_FORMAT_ARGS(ControlFile->checkPoint),
- ControlFile->checkPointCopy.ThisTimeLineID,
+ LSN_FORMAT_ARGS(CheckPointLoc),
+ CheckPointTLI,
LSN_FORMAT_ARGS(switchpoint))));
}
--
2.34.1
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-01-24 06:44 Michael Paquier <michael@paquier.xyz>
parent: David Steele <david@pgbackrest.org>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-01-24 06:44 UTC (permalink / raw)
To: David Steele <david@pgbackrest.org>; +Cc: pgsql-hackers
On Thu, Jan 23, 2025 at 02:53:39PM +0000, David Steele wrote:
> 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
Indeed, that's confusing. There is nothing telling you what's
actually wrong here, particularly if there is a typo of any kind in
the parameter.
> I discovered this while testing on Postgres versions < 12 where
> The tests are probably excessive but I needed something to show that the
> verification works as expected.
Even with your patch, specifying an incorrect name results in a
complaint about a timeline of 0. Wouldn't it be better to strengthen
the parsing in check_recovery_target_timeline() and/or the error
message reported?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-01-24 13:36 David Steele <david@pgbackrest.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: David Steele @ 2025-01-24 13:36 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 1/24/25 01:44, Michael Paquier wrote:
> On Thu, Jan 23, 2025 at 02:53:39PM +0000, David Steele wrote:
>
>> I discovered this while testing on Postgres versions < 12 where
>> The tests are probably excessive but I needed something to show that the
>> verification works as expected.
>
> Even with your patch, specifying an incorrect name results in a
> complaint about a timeline of 0. Wouldn't it be better to strengthen
> the parsing in check_recovery_target_timeline() and/or the error
> message reported?
I attached the wrong patch. Oops!
Regards,
-David
Attachments:
timeline-check-v1.patch (text/plain, 4.2K) download
From 453c8937868e0c70e4bfecf0a39bf8052e379efe Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Thu, 23 Jan 2025 14:40:43 +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 | 14 ++++-
src/test/recovery/t/003_recovery_targets.pl | 60 +++++++++++++++++++++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index cf2b007806f..1941e3061f4 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -4973,15 +4973,25 @@ check_recovery_target_timeline(char **newval, void **extra, GucSource source)
rttg = RECOVERY_TARGET_TIMELINE_LATEST;
else
{
+ char *endp;
+ unsigned long long timeline;
rttg = RECOVERY_TARGET_TIMELINE_NUMERIC;
errno = 0;
- strtoul(*newval, NULL, 0);
- if (errno == EINVAL || errno == ERANGE)
+ 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;
}
+
+ if (timeline < 2 || timeline > UINT_MAX)
+ {
+ GUC_check_errdetail(
+ "\"recovery_target_timeline\" must be between 2 and 4,294,967,295.");
+ return false;
+ }
}
myextra = (RecoveryTargetTimeLineGoal *) guc_malloc(ERROR, sizeof(RecoveryTargetTimeLineGoal));
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 0ae2e982727..0424ff4ad22 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -187,4 +187,64 @@ 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');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/is not a valid number/,
+ 'invalid target timelime');
+
+# Timeline target out of min range
+$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_timeline = '1'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery startup fails');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/must be between 2 and 4,294,967,295/,
+ 'target timelime out of min range');
+
+# Timeline target out of max range
+$node_standby = PostgreSQL::Test::Cluster->new('standby_11');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$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');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/must be between 2 and 4,294,967,295/,
+ 'target timelime out of max range');
+
done_testing();
--
2.34.1
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-02-14 07:42 Michael Paquier <michael@paquier.xyz>
parent: David Steele <david@pgbackrest.org>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-02-14 07:42 UTC (permalink / raw)
To: David Steele <david@pgbackrest.org>; +Cc: pgsql-hackers
On Fri, Jan 24, 2025 at 01:36:45PM +0000, David Steele wrote:
> I attached the wrong patch. Oops!
Thanks for the new patch.
> + 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).
> +
> + 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.
> +$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.
- 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?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-04-24 15:34 David Steele <david@pgbackrest.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: David Steele @ 2025-04-24 15:34 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
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
Attachments:
timeline-check-v2.patch (text/plain, 3.8K) download
From 2ccf6f207a14dafe1a426208618ae7690c4513a2 Mon Sep 17 00:00:00 2001
From: David Steele <david@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
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-04-25 00:12 Michael Paquier <michael@paquier.xyz>
parent: David Steele <david@pgbackrest.org>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2025-04-25 00:12 UTC (permalink / raw)
To: David Steele <david@pgbackrest.org>; +Cc: pgsql-hackers
On Thu, Apr 24, 2025 at 03:34:29PM +0000, David Steele wrote:
> 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.
One thing that I have double-checked is if we have similar messages
for GUCs that are defined as strings while requiring some range
checks. While we have similar messages (tablesample code is one,
opclass a second), we don't have similar thing for GUCs. I'd bet that
this would be a win in the long-run anyway.
> 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.
On my laptop, 003_recovery_targets.pl increases from 8.2s to 8.7s,
which seems OK here for the coverage we have.
Undoing the changes in xlogrecovery.c causes the tests to fail, so we
are good.
"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.
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..
And you are following the fat-comma convention for the command lines,
thanks for doing that. 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.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-04-25 13:50 David Steele <david@pgbackrest.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: David Steele @ 2025-04-25 13:50 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
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.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
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-07-03 02:24 Michael Paquier <michael@paquier.xyz>
parent: David Steele <david@pgbackrest.org>
0 siblings, 2 replies; 10+ messages in thread
From: Michael Paquier @ 2025-07-03 02:24 UTC (permalink / raw)
To: David Steele <david@pgbackrest.org>; +Cc: pgsql-hackers
On Fri, Apr 25, 2025 at 01:50:16PM +0000, David Steele wrote:
> That was my expectation. I just had some time to get this patch updated so
> took the opportunity.
And well, now is the time to get this patch done. So, applied after
an extra lookup, with a switch from UINT_MAX to PG_UINT32_MAX in the
code.
Thanks for your patience!
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Improve verification of recovery_target_timeline GUC.
@ 2025-07-04 13:39 David Steele <david@pgbackrest.org>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 10+ messages in thread
From: David Steele @ 2025-07-04 13:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers
On 7/2/25 22:24, Michael Paquier wrote:
> On Fri, Apr 25, 2025 at 01:50:16PM +0000, David Steele wrote:
>> That was my expectation. I just had some time to get this patch updated so
>> took the opportunity.
>
> And well, now is the time to get this patch done. So, applied after
> an extra lookup, with a switch from UINT_MAX to PG_UINT32_MAX in the
> code.
Excellent, thank you! I'm also planning a similar improvement for
check_recovery_target_xid as we discussed up thread.
Regards,
-David
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Improve verification of recovery_target_timeline GUC.
@ 2026-02-20 03:38 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 10+ messages in thread
From: Michael Paquier @ 2026-02-20 03:38 UTC (permalink / raw)
To: David Steele <david@pgbackrest.org>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Fri, Feb 20, 2026 at 03:25:30AM +0000, David Steele wrote:
> I noticed that when you replaced UINT_MAX with PG_UINT32_MAX you missed one
> instance. Fixed in the attached.
Added pgsql-hackers in CC for awareness, with your proposed patch
attached, David.
It looks so. Will fix, thanks!
--
Michael
Attachments:
uint32.patch (text/plain, 581B) download
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c0c2744d45b..fc794693442 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -5070,7 +5070,7 @@ check_recovery_target_timeline(char **newval, void **extra, GucSource source)
if (timeline < 1 || timeline > PG_UINT32_MAX)
{
GUC_check_errdetail("\"%s\" must be between %u and %u.",
- "recovery_target_timeline", 1, UINT_MAX);
+ "recovery_target_timeline", 1, PG_UINT32_MAX);
return false;
}
}
end of thread, other threads:[~2026-02-20 03:38 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-01-23 14:53 Improve verification of recovery_target_timeline GUC. David Steele <david@pgbackrest.org>
2025-01-24 06:44 ` Michael Paquier <michael@paquier.xyz>
2025-01-24 13:36 ` David Steele <david@pgbackrest.org>
2025-02-14 07:42 ` Michael Paquier <michael@paquier.xyz>
2025-04-24 15:34 ` David Steele <david@pgbackrest.org>
2025-04-25 00:12 ` Michael Paquier <michael@paquier.xyz>
2025-04-25 13:50 ` David Steele <david@pgbackrest.org>
2025-07-03 02:24 ` Michael Paquier <michael@paquier.xyz>
2025-07-04 13:39 ` David Steele <david@pgbackrest.org>
2026-02-20 03:38 ` Michael Paquier <michael@paquier.xyz>
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