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