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
signature.asc
Description: PGP signature
