MIME-Version: 1.0
References: 
 <CAM527d9exRCdWrhJOnAxk_vACg7sr_yPoaJp_+uCFY0qP8v=aw@mail.gmail.com>
 <CA+HiwqGTOwRqkgrhqq6-nLyVGfGuAHMfoo+Ob2A4Z98ZkgwCmg@mail.gmail.com>
 <CA+HiwqGWUtxc7KECuT06aYTiwwxGBxM89qY_W64dQjYEoziXog@mail.gmail.com>
 <CA+HiwqHUz50YqJn4XiNsSLN2c+9eYBy1af=y_dfdJTsz5BmbJg@mail.gmail.com>
 <CAM527d_2OpJ3KCOT1QqGh4neCPpgZTgM+VUxTqVgOSweOzTDQw@mail.gmail.com>
 <CA+HiwqFBXTTy3KcfGVxqxkhX5zV99R7=s2EwkxMiiWnVbyTpyw@mail.gmail.com>
 <CAJTYsWVgHCNDZb2F62F+aELnKJO2BWtHaAXcN-AmgPPP+GAnUQ@mail.gmail.com>
 <CA+HiwqELE-eyOfBBEmpr_eGf-04PUvZg5BjypW2CMHbed5QGhA@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqELE-eyOfBBEmpr_eGf-04PUvZg5BjypW2CMHbed5QGhA@mail.gmail.com>
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Thu, 11 Jun 2026 16:18:15 +0800
Message-ID: 
 <CAEG8a3JhZwvdeedTvQCvvXgVkeb6ENW2haiHfxnXkXbv3NMmjg@mail.gmail.com>
Subject: Re: PG19 FK fast path: OOB write and missed FK checks during batched
To: Amit Langote <amitlangote09@gmail.com>
Cc: Ayush Tiwari <ayushtiwari.slg01@gmail.com>,
 Nikolay Samokhvalov <nik@postgres.ai>,
	pgsql-hackers mailing list <pgsql-hackers@postgresql.org>,
 Andrey Borodin <amborodin@acm.org>,
	Kirk Wolak <wolakk@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAEG8a3JhZwvdeedTvQCvvXgVkeb6ENW2haiHfxnXkXbv3NMmjg%40mail.gmail.com>
Precedence: bulk

Hi Amit,

On Wed, Jun 10, 2026 at 8:17=E2=80=AFPM Amit Langote <amitlangote09@gmail.c=
om> wrote:
>
> Hi Ayush,
>
> Thanks for the review.
>
> On Wed, Jun 10, 2026 at 7:09=E2=80=AFPM Ayush Tiwari
> <ayushtiwari.slg01@gmail.com> wrote:
> > On Wed, 10 Jun 2026 at 14:02, Amit Langote <amitlangote09@gmail.com> wr=
ote
> >> 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 coup=
le 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-wid=
e
> > ri_fastpath_flushing guard already routes the re-entrant check to the p=
er-row
> > path before it gets back into ri_FastPathBatchAdd().  Does that mean th=
e
> > 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_FastPathBatch=
Add()
> > 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 switchi=
ng the
> > test to 65 rows, or adding that variant, so the per-entry guard is cove=
red 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_cach=
e and
> > clears ri_fastpath_callback_registered at transaction end, I wondered w=
hether
> > it might be worth clearing ri_fastpath_flushing there too, just as chea=
p
> > insurance against some future path that leaves it set across transactio=
ns
> > 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.

I only reviewed and applied patch 0001 on my local machine, and it
successfully fixed the crash.

One minor comment:

+ if (fpentry->flushing)
+ {
+ ri_FastPathCheck(riinfo, fk_rel, newslot);
+ return;
+ }

Would it be worth wrapping the condition with unlikely()? It seems
this branch is expected to be false in most cases, not a strong
opinion though.

>
> --
> Thanks, Amit Langote


--=20
Regards
Junwang Zhao