Re: pg_walinspect: fix LSN validation messages and empty range handling - Mailing list pgsql-hackers

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