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

👍

Kiran Kaki reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>

On Tue, Sep 22, 2026 at 11:10 AM surya poondla <suryapoondla4(at)gmail(dot)com>
wrote:

> 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
>