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