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
signature.asc
Description: PGP signature
