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
