Improve verification of recovery_target_timeline GUC.

Started by David Steele12 months ago9 messages
#1David Steele
david@pgbackrest.org
1 attachment(s)

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.patchtext/plain; charset=UTF-8; name=timeline-log-v1.patchDownload
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

#2Michael Paquier
michael@paquier.xyz
In reply to: David Steele (#1)
Re: Improve verification of recovery_target_timeline GUC.

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

#3David Steele
david@pgbackrest.org
In reply to: Michael Paquier (#2)
1 attachment(s)
Re: Improve verification of recovery_target_timeline GUC.

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.patchtext/plain; charset=UTF-8; name=timeline-check-v1.patchDownload
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

#4Michael Paquier
michael@paquier.xyz
In reply to: David Steele (#3)
Re: Improve verification of recovery_target_timeline GUC.

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

#5David Steele
david@pgbackrest.org
In reply to: Michael Paquier (#4)
1 attachment(s)
Re: Improve verification of recovery_target_timeline GUC.

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.patchtext/plain; charset=UTF-8; name=timeline-check-v2.patchDownload
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

#6Michael Paquier
michael@paquier.xyz
In reply to: David Steele (#5)
Re: Improve verification of recovery_target_timeline GUC.

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

#7David Steele
david@pgbackrest.org
In reply to: Michael Paquier (#6)
1 attachment(s)
Re: Improve verification of recovery_target_timeline GUC.

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.patchtext/plain; charset=UTF-8; name=timeline-check-v3.patchDownload
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

#8Michael Paquier
michael@paquier.xyz
In reply to: David Steele (#7)
Re: Improve verification of recovery_target_timeline GUC.

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

#9David Steele
david@pgbackrest.org
In reply to: Michael Paquier (#8)
Re: Improve verification of recovery_target_timeline GUC.

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