| From: | Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> |
|---|---|
| To: | Amit Langote <amitlangote09(at)gmail(dot)com> |
| Cc: | Nikolay Samokhvalov <nik(at)postgres(dot)ai>, pgsql-hackers mailing list <pgsql-hackers(at)postgresql(dot)org>, Andrey Borodin <amborodin(at)acm(dot)org>, Kirk Wolak <wolakk(at)gmail(dot)com> |
| Subject: | Re: PG19 FK fast path: OOB write and missed FK checks during batched |
| Date: | 2026-06-10 10:08:53 |
| Message-ID: | CAJTYsWVgHCNDZb2F62F+aELnKJO2BWtHaAXcN-AmgPPP+GAnUQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09(at)gmail(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Japin Li | 2026-06-10 10:22:01 | Re: Fix md5_password_warnings for role/database settings |
| Previous Message | Wolfgang Walther | 2026-06-10 09:43:07 | Re: PRI?64 vs Visual Studio (2022) |