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