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: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Wed, 10 Jun 2026 18:18:59 +0530
Message-ID: 
 <CAJTYsWXPhhNeC08kGQRqc3snBW9i0jL6Sr2s2XwbT9WwBrA7Gw@mail.gmail.com>
Subject: Re: PG19 FK fast path: OOB write and missed FK checks during batched
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>
Content-Type: multipart/alternative; boundary="00000000000094a5c60653e5ab21"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWXPhhNeC08kGQRqc3snBW9i0jL6Sr2s2XwbT9WwBrA7Gw%40mail.gmail.com>
Precedence: bulk

--00000000000094a5c60653e5ab21
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, 10 Jun 2026 at 17:47, Amit Langote <amitlangote09@gmail.com> 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>
> 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-wid=
e
> > 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 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_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.
>

Makes sense, it is better.

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

Thanks for the updated patches!

Both patches, lgtm.

Regards,
Ayush

--00000000000094a5c60653e5ab21
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, 10 Jun 2=
026 at 17:47, Amit Langote &lt;<a href=3D"mailto:amitlangote09@gmail.com">a=
mitlangote09@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_q=
uote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,2=
04);padding-left:1ex">Hi Ayush,<br>
<br>
Thanks for the review.<br>
<br>
On Wed, Jun 10, 2026 at 7:09=E2=80=AFPM Ayush Tiwari<br>
&lt;<a href=3D"mailto:ayushtiwari.slg01@gmail.com" target=3D"_blank">ayusht=
iwari.slg01@gmail.com</a>&gt; wrote:<br>
&gt; On Wed, 10 Jun 2026 at 14:02, Amit Langote &lt;<a href=3D"mailto:amitl=
angote09@gmail.com" target=3D"_blank">amitlangote09@gmail.com</a>&gt; wrote=
<br>
&gt;&gt; Thanks for checking.=C2=A0 I will review them a bit more closely b=
efore<br>
&gt;&gt; committing by Friday.=C2=A0 Other reviews are welcome.<br>
&gt;<br>
&gt; Thanks for the patch!<br>
&gt;<br>
&gt; I read through v1-0001 and v1-0002 and tried them locally. I had a cou=
ple of<br>
&gt; things I wanted to ask about.<br>
&gt;<br>
&gt; 1. The per-entry &quot;flushing&quot; flag and test coverage.=C2=A0 If=
 I&#39;m reading the two<br>
&gt; patches together correctly, with both applied the 64-row re-entry test=
 in 0001<br>
&gt; reaches the flush through ri_FastPathEndBatch(), where 0002&#39;s cach=
e-wide<br>
&gt; ri_fastpath_flushing guard already routes the re-entrant check to the =
per-row<br>
&gt; path before it gets back into ri_FastPathBatchAdd().=C2=A0 Does that m=
ean the<br>
&gt; per-entry flag from 0001 isn&#39;t really exercised by that test once =
0002 is in?<br>
&gt; As far as I can tell you&#39;d need the flush to fire from ri_FastPath=
BatchAdd()<br>
&gt; itself (a 65th row) to reach it.=C2=A0 I tried a 65-row variant (same =
FK, re-entrant<br>
&gt; DML from the cast during the full-batch flush), including a case where=
 the<br>
&gt; re-entrant row was an orphan, and it seemed to do the right thing; the=
<br>
&gt; per-row fallback still raised the violation.=C2=A0 Would it be worth s=
witching the<br>
&gt; test to 65 rows, or adding that variant, so the per-entry guard is cov=
ered too?<br>
&gt; Or am I missing a path where the committed test already hits it?<br>
<br>
You&#39;re right. With 0002 applied, the 64-row test reaches the flush<br>
through ri_FastPathEndBatch(), where the cache-wide<br>
ri_fastpath_flushing guard catches the re-entry before it returns to<br>
ri_FastPathBatchAdd(), so the per-entry flag is no longer exercised by<br>
that test. To hit the per-entry flag the flush has to fire from<br>
ri_FastPathBatchAdd() itself, which the 64-row case no longer does<br>
once the add and flush are reordered.<br>
<br>
Rather than bump the test to 65 rows, I&#39;d prefer to keep the flush<br>
firing from ri_FastPathBatchAdd() at 64 by not reordering the add and<br>
flush, and prevent the OOB write by bounds-checking the write instead,<br>
as done in the attached updated 0001. A re-entrant add then can&#39;t<br>
overrun the array regardless of the flag, the per-entry flushing guard<br>
still routes the re-entry to the per-row path, and a 64-row statement<br>
flushes from ri_FastPathBatchAdd() on the 64th row, so the existing<br>
test exercises the per-entry guard.<br></blockquote><div><br></div><div>Mak=
es sense, it is better.<br><br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">
&gt; 2. Resetting ri_fastpath_flushing.=C2=A0 I noticed it&#39;s cleared on=
ly in the<br>
&gt; PG_FINALLY of ri_FastPathEndBatch(), which does seem to cover the case=
s I could<br>
&gt; think of.=C2=A0 Since ri_FastPathXactCallback already NULLs ri_fastpat=
h_cache and<br>
&gt; clears ri_fastpath_callback_registered at transaction end, I wondered =
whether<br>
&gt; it might be worth clearing ri_fastpath_flushing there too, just as che=
ap<br>
&gt; insurance against some future path that leaves it set across transacti=
ons<br>
&gt; though maybe that&#39;s unnecessary given the PG_FINALLY.<br>
<br>
Agreed, it&#39;s cheap and matches the existing resets there, so I&#39;ve<b=
r>
added it to ri_FastPathXactCallback() in v2-0002.<br>
<br>
&gt; Other than the above queries, the patch looks good to me.<br>
<br>
Updated patches attached.<br></blockquote><div><br>Thanks for the updated p=
atches!<br><br>Both patches, lgtm.<br><br>Regards,<br>Ayush=C2=A0</div></di=
v></div>

--00000000000094a5c60653e5ab21--