Re: PG19 FK fast path: OOB write and missed FK checks during batched
Ayush Tiwari <ayushtiwari.slg01@gmail.com>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Nikolay Samokhvalov <nik@postgres.ai>, pgsql-hackers mailing list <pgsql-hackers@postgresql.org>,
Andrey Borodin <amborodin@acm.org>, Kirk Wolak <wolakk@gmail.com>
Date: 2026-06-10T10:08:53Z
Lists: pgsql-hackers
Hi, On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com> wrote > > Thanks for checking. I will review them a bit more closely before > committing by Friday. Other reviews are welcome. > Thanks for the patch! I read through v1-0001 and v1-0002 and tried them locally. I had a couple of things I wanted to ask about. 1. The per-entry "flushing" flag and test coverage. If I'm reading the two patches together correctly, with both applied the 64-row re-entry test in 0001 reaches the flush through ri_FastPathEndBatch(), where 0002's cache-wide ri_fastpath_flushing guard already routes the re-entrant check to the per-row path before it gets back into ri_FastPathBatchAdd(). Does that mean the per-entry flag from 0001 isn't really exercised by that test once 0002 is in? As far as I can tell you'd need the flush to fire from ri_FastPathBatchAdd() itself (a 65th row) to reach it. I tried a 65-row variant (same FK, re-entrant DML from the cast during the full-batch flush), including a case where the re-entrant row was an orphan, and it seemed to do the right thing; the per-row fallback still raised the violation. Would it be worth switching the test to 65 rows, or adding that variant, so the per-entry guard is covered too? Or am I missing a path where the committed test already hits it? 2. Resetting ri_fastpath_flushing. I noticed it's cleared only in the PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the cases I could think of. Since ri_FastPathXactCallback already NULLs ri_fastpath_cache and clears ri_fastpath_callback_registered at transaction end, I wondered whether it might be worth clearing ri_fastpath_flushing there too, just as cheap insurance against some future path that leaves it set across transactions though maybe that's unnecessary given the PG_FINALLY. Other than the above queries, the patch looks good to me. Regards, Ayush
Commits
-
Confine RI fast-path batching to the top transaction level
- 4113873a5ab0 19 (unreleased) landed
-
Fix out-of-bounds write in RI fast-path batch on re-entry
- 0e47bb5fbeec 19 (unreleased) landed
-
Stamp 19beta1.
- 4b0bf0788b06 19 (unreleased) cited