Re: pg_walinspect: fix LSN validation messages and empty range handling

Hi Chao,

Thanks for the patches. I reviewed and tested v4 on master (e8a3ee5b197).
Both apply cleanly with git am, build without new
warnings, and the below things pass:
- contrib/pg_walinspect: 2/2 (pg_walinspect, oldextversions)
- src/test/regress: 239/239, including subscription
- src/bin/psql TAP: 4 files, 237 tests, including 001_basic.pl

0001 looks good to me, no comments. A few things I checked specifically:
1) All five affected messages in pg_walinspect.c are covered (lines 491,
533, 540, 839 and 858 after the patch).
Grepping the tree for the old wording across *.c, *.out, *.sql and *.sgml
turns up nothing, so no expected file or doc text is left stale.
2) The characterisation of 5c1b6628075a is accurate: it really did change
"lsn >= *curr_lsn" to "lsn > curr_lsn" and
"start_lsn >= end_lsn" to "start_lsn > *end_lsn" while only rewording the
messages.
3) pgwalinspect.sgml never states the strict relation, so no doc change is
owed here.
4) Both new wordings have precedent, which matches Bharath's preference
upthread: "must be less than or equal to" appears in
pseudorandomfuncs.c, rangetypes.c and numeric.c, and "must be greater than
or equal to" in pg_upgrade/check.c, pg_resetwal.c
and xlogrecovery.c.

0002 also looks correct to me. Both message changes match the conditions
they guard, and the two new failure-case tests pin the new
wording, so they would not pass against the old messages.
The comments below are all about the test additions rather than the fix
itself; none of them affect correctness, so please treat them as
suggestions.

1. In subscription.sql, I don't think the session-authorization is needed.
regress_subscription_user is created as LOGIN SUPERUSER at the top of the
file and is the active role
from line 190 onwards, so the session is already superuser when the new
block runs.
I removed the RESET/SET pair locally and the subscription test still
passes, so the superuser bypass over
pg_replication_origin_advance's restricted ACL is enough.
Dropping them also keeps the new test running as the subscription owner,
which is what every other SKIP test in the file does.

2. The first comment in the new block looks misplaced:
-- fail - LSN must not be behind the replication origin
RESET SESSION AUTHORIZATION;

RESET SESSION AUTHORIZATION neither fails nor has anything to do with the
origin, and the comment duplicates the one further down above the statement
it actually describes. I think it can just be removed.

3. Still in that block, pg_replication_origin_advance() returns
void, so its output is stable across runs and there is nothing for \gset to
suppress. A plain terminated statement would read more naturally and
wouldn't leave a psql variable behind.

4. Very minor: the new WATCH_INTERVAL test uses

'\set WATCH_INTERVAL 1000000' . "\n" . '\echo :WATCH_INTERVAL'
where the surrounding tests in 001_basic.pl use a literal multiline
single-quoted string.

Finally, on Bharath's question upthread about whether anything else needs
the same treatment: I went through the remaining
"must be less than" / "must be greater than" messages in the tree and each
one is consistent with the condition it guards, so I believe the
change is complete. The ones I looked at were postinit.c, postmaster.c,
partbounds.c, parse_utilcmd.c, sequence.c, wparser_def.c and pl_exec.c.

Regards,
Surya Poondla