| From: | Kiran Kaki <itskkpg(at)gmail(dot)com> |
|---|---|
| To: | surya poondla <suryapoondla4(at)gmail(dot)com> |
| Cc: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz> |
| Subject: | Re: pg_walinspect: fix LSN validation messages and empty range handling |
| Date: | 2026-09-22 21:35:24 |
| Message-ID: | CAD0dvCRAYikegVUWbmDaHuWa9iKRP7q4VXaBP4h6c+WK4Cpptg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Surya, Chao
>> PFA v4:
>>
>> * 0001 unchanged
>> * 0002 added the tests that Kiran suggested
Thanks for the review, Surya. I tested v4 on the same base
(e8a3ee5b197) and results are inline with your
findings
> 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.
Agree on this!
> 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.
Agree on this!
> 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.
Agree on 3. The \gset here is only hiding an empty one-row result, and
a plain statement says what it means. Worth noting it adds five lines to
the expected file, which seems a fair trade.
> 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.
Point 4 is my mistake, not Chao's. That concatenation came from the
snippet I sent upthread and he may have used it as-is. Sorry for the noise.
Thanks,
Kiran Kaki.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Merlin Moncure | 2026-09-22 21:41:26 | Re: Up to 50x degradation in dblink performance when receiving notice traffic 19 vs 18 |
| Previous Message | Corey Huinker | 2026-09-22 21:21:25 | Re: proposal: add new dblink function, dblink_wait_for_query() |