Thread: 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
Attachment
On Thu, Apr 24, 2025 at 03:34:29PM +0000, David Steele wrote: > 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. One thing that I have double-checked is if we have similar messages for GUCs that are defined as strings while requiring some range checks. While we have similar messages (tablesample code is one, opclass a second), we don't have similar thing for GUCs. I'd bet that this would be a win in the long-run anyway. > 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. On my laptop, 003_recovery_targets.pl increases from 8.2s to 8.7s, which seems OK here for the coverage we have. Undoing the changes in xlogrecovery.c causes the tests to fail, so we are good. "invalid recovery startup fails" is used three times repeatedly for the tests with bogus and out-of-bound values. I'd suggest to make these more verbose, with three extras "bogus value", "lower bound check" and "upper bound check" added. Side thing noted while reading the area: check_recovery_target_xid() does not have any GUC_check_errdetail() giving details for the EINVAL and ERANGE cases. Your addition for recovery_target_timeline is a good idea, so perhaps we could do the same there? No need to do that, that's just something I've noticed in passing.. And you are following the fat-comma convention for the command lines, thanks for doing that. Note that I'm not planning to do anything here for v18 now that we are in feature freeze, these would be for v19 once the branch opens. -- Michael
Attachment
On 4/24/25 20:12, Michael Paquier wrote: > On Thu, Apr 24, 2025 at 03:34:29PM +0000, David Steele wrote: >> >> 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. > > On my laptop, 003_recovery_targets.pl increases from 8.2s to 8.7s, > which seems OK here for the coverage we have. Multiply half a second by similar tests on all the GUCs and it would add up to a lot. But no argument here on keeping the tests. > "invalid recovery startup fails" is used three times repeatedly for > the tests with bogus and out-of-bound values. I'd suggest to make > these more verbose, with three extras "bogus value", "lower bound > check" and "upper bound check" added. I have updated the labels to be more descriptive. > Side thing noted while reading the area: check_recovery_target_xid() > does not have any GUC_check_errdetail() giving details for the EINVAL > and ERANGE cases. Your addition for recovery_target_timeline is a > good idea, so perhaps we could do the same there? No need to do that, > that's just something I've noticed in passing.. I don't want to add that to this commit but I have added a note to my list of PG improvements to make when I have time. > And you are following the fat-comma convention for the command lines, > thanks for doing that. Just trying to follow the convention from existing tests, but you are welcome! > Note that I'm not planning to do anything here > for v18 now that we are in feature freeze, these would be for v19 once > the branch opens. That was my expectation. I just had some time to get this patch updated so took the opportunity. Regards, -David