| From: | Kiran Kaki <itskkpg(at)gmail(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz> |
| Subject: | Re: pg_walinspect: fix LSN validation messages and empty range handling |
| Date: | 2026-09-22 02:06:49 |
| Message-ID: | CAD0dvCQqQB36w2HCjza_PRG=eTUsBipJ1126J=BJQvm3sM3knQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi @Chao Li
On Mon, Sep 21, 2026 at 3:51 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> Thanks for reviewing and updating v2.
>
> >
> > While here, do we also need to fix AlterSubscription()'s skip WAL
> > location and ParseVariableDouble()'s min and max bound messages? Maybe
> > separately.
> >
>
> Yeah, we can do that. As those two functions are in core, and 0001 changes the extension, I put the new changes to 0002.
>
> PFA v3.
Tested both patches on master (d39fda1cc4). They apply cleanly, build
without warnings, and the full suite passes (363 ok, 0 failed). I also
confirmed each reworded message at runtime, including the inclusive
boundaries they describe.
0001 is unchanged since v1, only the commit message differs. Looks good to me.
For 0002, the comment above that check already says "at least a future
LSN" and came in with the code in 208c5d65bb, so the reworded message
matches what the check was always meant to do. Looks good to me.
One suggestion, not a blocker. The new tests only cover the cases that
fail. The lower bound is already covered by the existing "Test zero
interval" case in 001_basic.pl, but the upper bound and the equal LSN
case are not, so they would still pass if those bounds were made
exclusive. Two additions would close that:
psql_like(
$node,
'\set WATCH_INTERVAL 1000000' . "\n" . '\echo :WATCH_INTERVAL',
qr/^1000000$/m,
'WATCH_INTERVAL accepts its upper bound');
-- ok - LSN equal to the replication origin is accepted
ALTER SUBSCRIPTION regress_testsub SKIP (lsn = '0/12346');
ALTER SUBSCRIPTION regress_testsub SKIP (lsn = NONE);
The second line just resets the value so the rest of the file's expected
output is unaffected.
Both patches are correct as they stand, so this is ready for a committer
either way. The test additions are optional, so please see if you want
to take them before committing.
Thanks Bharath for the review comments upthread, they made the scope of
these changes much easier to follow.
Regards,
Kiran Kaki
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Wei Sun | 2026-09-22 02:15:29 | Re: Severe performance degradation with concurrent updates due to excessive EvalPlanQual (EPQ) re‑evaluation |
| Previous Message | Manu | 2026-09-22 02:01:49 | Re: [PATCH v1] [BUG #19507] Prevent constraint name conflicts in partition trees spanning multiple schemas |