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>
In-Reply-To: 
 <CA+HiwqFBXTTy3KcfGVxqxkhX5zV99R7=s2EwkxMiiWnVbyTpyw@mail.gmail.com>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Wed, 10 Jun 2026 15:38:53 +0530
Message-ID: 
 <CAJTYsWVgHCNDZb2F62F+aELnKJO2BWtHaAXcN-AmgPPP+GAnUQ@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="000000000000f5a70c0653e36e8f"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWVgHCNDZb2F62F%2BaELnKJO2BWtHaAXcN-AmgPPP%2BGAnUQ%40mail.gmail.com>
Precedence: bulk

--000000000000f5a70c0653e36e8f
Content-Type: text/plain; charset="UTF-8"

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

--000000000000f5a70c0653e36e8f
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 14:02, Amit Langote &lt;<a href=3D"mailto:amitlangote09@gmail.com">a=
mitlangote09@gmail.com</a>&gt; wrote</div><blockquote class=3D"gmail_quote"=
 style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);p=
adding-left:1ex">
<br>
Thanks for checking.=C2=A0 I will review them a bit more closely before<br>
committing by Friday.=C2=A0 Other reviews are welcome.<br></blockquote><div=
><br>Thanks for the patch!=C2=A0</div><div><br>I read through v1-0001 and v=
1-0002 and tried them locally.=C2=A0I had a couple of<br>things I wanted to=
 ask about.<br><br>1. The per-entry &quot;flushing&quot; flag and test cove=
rage.=C2=A0 If I&#39;m reading the two<br>patches together correctly, with =
both applied the 64-row re-entry test in 0001<br>reaches the flush through =
ri_FastPathEndBatch(), where 0002&#39;s cache-wide<br>ri_fastpath_flushing =
guard already routes the re-entrant check to the per-row<br>path before it =
gets back into ri_FastPathBatchAdd().=C2=A0 Does that mean the<br>per-entry=
 flag from 0001 isn&#39;t really exercised by that test once 0002 is in?<br=
>As far as I can tell you&#39;d need the flush to fire from ri_FastPathBatc=
hAdd()<br>itself (a 65th row) to reach it.=C2=A0 I tried a 65-row variant (=
same FK, re-entrant<br>DML from the cast during the full-batch flush), incl=
uding a case where the<br>re-entrant row was an orphan, and it seemed to do=
 the right thing; the<br>per-row fallback still raised the violation.=C2=A0=
 Would it be worth switching the<br>test to 65 rows, or adding that variant=
, so the per-entry guard is covered too?<br>Or am I missing a path where th=
e committed test already hits it?<br><br>2. Resetting ri_fastpath_flushing.=
=C2=A0 I noticed it&#39;s cleared only in the<br>PG_FINALLY of ri_FastPathE=
ndBatch(), which does seem to cover the cases I could<br>think of.=C2=A0 Si=
nce ri_FastPathXactCallback already NULLs ri_fastpath_cache and<br>clears r=
i_fastpath_callback_registered at transaction end, I wondered whether<br>it=
 might be worth clearing ri_fastpath_flushing there too, just as cheap<br>i=
nsurance against some future path that leaves it set across transactions<br=
>though maybe that&#39;s unnecessary given the PG_FINALLY.<br><br>Other tha=
n the above queries, the patch looks good to me.<br><br>Regards,<br>Ayush<b=
r>=C2=A0</div></div></div>

--000000000000f5a70c0653e36e8f--