Re: PG19 FK fast path: OOB write and missed FK checks during batched

Hi Ayush,

Thanks for the review.

On Wed, Jun 10, 2026 at 7:09 PM Ayush Tiwari
<ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> 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?

You're right. With 0002 applied, the 64-row test reaches the flush
through ri_FastPathEndBatch(), where the cache-wide
ri_fastpath_flushing guard catches the re-entry before it returns to
ri_FastPathBatchAdd(), so the per-entry flag is no longer exercised by
that test. To hit the per-entry flag the flush has to fire from
ri_FastPathBatchAdd() itself, which the 64-row case no longer does
once the add and flush are reordered.

Rather than bump the test to 65 rows, I'd prefer to keep the flush
firing from ri_FastPathBatchAdd() at 64 by not reordering the add and
flush, and prevent the OOB write by bounds-checking the write instead,
as done in the attached updated 0001. A re-entrant add then can't
overrun the array regardless of the flag, the per-entry flushing guard
still routes the re-entry to the per-row path, and a 64-row statement
flushes from ri_FastPathBatchAdd() on the 64th row, so the existing
test exercises the per-entry guard.

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

Agreed, it's cheap and matches the existing resets there, so I've
added it to ri_FastPathXactCallback() in v2-0002.

> Other than the above queries, the patch looks good to me.

Updated patches attached.

--
Thanks, Amit Langote