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