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
