Re: Improve verification of recovery_target_timeline GUC.

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