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>
In-Reply-To: 
 <CA+HiwqHUz50YqJn4XiNsSLN2c+9eYBy1af=y_dfdJTsz5BmbJg@mail.gmail.com>
From: Nikolay Samokhvalov <nik@postgres.ai>
Date: Wed, 10 Jun 2026 01:16:16 -0700
Message-ID: 
 <CAM527d_2OpJ3KCOT1QqGh4neCPpgZTgM+VUxTqVgOSweOzTDQw@mail.gmail.com>
Subject: Re: PG19 FK fast path: OOB write and missed FK checks during batched
To: Amit Langote <amitlangote09@gmail.com>
Cc: pgsql-hackers mailing list <pgsql-hackers@postgresql.org>,
 Andrey Borodin <amborodin@acm.org>,
	Kirk Wolak <wolakk@gmail.com>
Content-Type: multipart/alternative; boundary="00000000000040c6190653e1dc64"
Archived-At: 
 <https://www.postgresql.org/message-id/CAM527d_2OpJ3KCOT1QqGh4neCPpgZTgM%2BVUxTqVgOSweOzTDQw%40mail.gmail.com>
Precedence: bulk

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

On Tue, Jun 9, 2026 at 6:31=E2=80=AFAM Amit Langote <amitlangote09@gmail.co=
m> wrote:

> On Mon, Jun 8, 2026 at 5:18=E2=80=AFPM Amit Langote <amitlangote09@gmail.=
com>
> wrote:
> > On Sat, Jun 6, 2026 at 6:13=E2=80=AFPM Amit Langote <amitlangote09@gmai=
l.com>
> wrote:
> > > Thanks for the detailed report and reproducers. I=E2=80=99ve started =
looking
> into this.
> >
> > Continuing to look.  Appended this to the open items list:
> >
> > https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues
>
> Thanks again, Nik, for the thorough analysis and the reproducers --
> they made all three easy to confirm and pin down. Patches attached:
> 0001 for defect 1, 0002 for defects 2 and 3.
>
> 0001 (defect 1): check and flush before writing the row rather than
> after, and add a per-entry "flushing" flag so a re-entrant add on the
> same entry during a flush takes the per-row path instead of touching
> the mid-flush batch. The flag is cleared in a PG_FINALLY, which also
> resets batch_count, so the entry stays reusable if a flush error is
> caught by a savepoint.
>
> 0002 (defects 2 and 3): rather than track subxact membership per row,
> confine batching to the top transaction level -- in RI_FKey_check,
> when GetCurrentTransactionNestLevel() > 1, use the per-row path. I
> went this way because per-entry subxact tracking isn't enough (one
> entry's batch can mix rows from several levels, since the cache is
> keyed by constraint), and flushing at subxact boundaries doesn't work
> for deferred constraints. Once the cache only ever holds top-level
> rows, a subxact abort has nothing of its own to discard, so
> ri_FastPathSubXactCallback goes away -- that's what fixes your defect
> 2 reproducer. For defect 3, which is still reachable at the top level,
> the same patch adds a cache-wide flag set while ri_FastPathEndBatch
> iterates, so a re-entrant check during the scan takes the per-row path
> instead of inserting into the cache being scanned.
>
> The per-row path still bypasses SPI, so these stay well ahead of the
> pre-19 check in terms of performance. I'd like to recover batching
> across subtransactions properly in v20 but didn't want to rush it now.
>
> On defect 3, can you check whether your reproducer still commits the
> orphan with 0002 applied, or whether (like on my build) it now raises
> the violation? I'd like to be sure the bucket-placement variation you
> hit is actually covered. And of course any review of the patches is
> welcome.
>
> --
> Thanks, Amit Langote
>

Hi Amit,

Thanks for the quick fixes.

I checked v1-0001 + v1-0002 against current master (e18b0cb7) with an
assertion/debug build.

- Both apply cleanly to master (in sequence)
- Defect 1 same-FK re-entry no longer crashes; the original shape completes
and leaves the expected rows
- Defect 2 subtransaction-abort case now raises the FK violation instead of
committing orphans
- For your defect 3 question: with 0002 applied, my reproducer no longer
commits the child2 orphan. It raises:
    ERROR: insert or update on table "child2" violates foreign key
constraint "child2_fkey"
    DETAIL: Key (a)=3D(999999) is not present in table "parent".

After the error, child2_orphans =3D 0 and child2 is empty in my run.

I also ran the regression suite in that tree; foreign_key passed, and the
full run reported all 245 tests passed.

So v1 looks good to me for the three reported cases.

Thanks!

Nik

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

<div dir=3D"ltr"><div dir=3D"ltr"><div>On Tue, Jun 9, 2026 at 6:31=E2=80=AF=
AM Amit Langote &lt;<a href=3D"mailto:amitlangote09@gmail.com">amitlangote0=
9@gmail.com</a>&gt; wrote:</div></div><div class=3D"gmail_quote gmail_quote=
_container"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0=
.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Jun 8=
, 2026 at 5:18=E2=80=AFPM Amit Langote &lt;<a href=3D"mailto:amitlangote09@=
gmail.com" target=3D"_blank">amitlangote09@gmail.com</a>&gt; wrote:<br>
&gt; On Sat, Jun 6, 2026 at 6:13=E2=80=AFPM Amit Langote &lt;<a href=3D"mai=
lto:amitlangote09@gmail.com" target=3D"_blank">amitlangote09@gmail.com</a>&=
gt; wrote:<br>
&gt; &gt; Thanks for the detailed report and reproducers. I=E2=80=99ve star=
ted looking into this.<br>
&gt;<br>
&gt; Continuing to look.=C2=A0 Appended this to the open items list:<br>
&gt;<br>
&gt; <a href=3D"https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#O=
pen_Issues" rel=3D"noreferrer" target=3D"_blank">https://wiki.postgresql.or=
g/wiki/PostgreSQL_19_Open_Items#Open_Issues</a><br>
<br>
Thanks again, Nik, for the thorough analysis and the reproducers --<br>
they made all three easy to confirm and pin down. Patches attached:<br>
0001 for defect 1, 0002 for defects 2 and 3.<br>
<br>
0001 (defect 1): check and flush before writing the row rather than<br>
after, and add a per-entry &quot;flushing&quot; flag so a re-entrant add on=
 the<br>
same entry during a flush takes the per-row path instead of touching<br>
the mid-flush batch. The flag is cleared in a PG_FINALLY, which also<br>
resets batch_count, so the entry stays reusable if a flush error is<br>
caught by a savepoint.<br>
<br>
0002 (defects 2 and 3): rather than track subxact membership per row,<br>
confine batching to the top transaction level -- in RI_FKey_check,<br>
when GetCurrentTransactionNestLevel() &gt; 1, use the per-row path. I<br>
went this way because per-entry subxact tracking isn&#39;t enough (one<br>
entry&#39;s batch can mix rows from several levels, since the cache is<br>
keyed by constraint), and flushing at subxact boundaries doesn&#39;t work<b=
r>
for deferred constraints. Once the cache only ever holds top-level<br>
rows, a subxact abort has nothing of its own to discard, so<br>
ri_FastPathSubXactCallback goes away -- that&#39;s what fixes your defect<b=
r>
2 reproducer. For defect 3, which is still reachable at the top level,<br>
the same patch adds a cache-wide flag set while ri_FastPathEndBatch<br>
iterates, so a re-entrant check during the scan takes the per-row path<br>
instead of inserting into the cache being scanned.<br>
<br>
The per-row path still bypasses SPI, so these stay well ahead of the<br>
pre-19 check in terms of performance. I&#39;d like to recover batching<br>
across subtransactions properly in v20 but didn&#39;t want to rush it now.<=
br>
<br>
On defect 3, can you check whether your reproducer still commits the<br>
orphan with 0002 applied, or whether (like on my build) it now raises<br>
the violation? I&#39;d like to be sure the bucket-placement variation you<b=
r>
hit is actually covered. And of course any review of the patches is<br>
welcome.<br>
<br>
--<br>
Thanks, Amit Langote<br></blockquote><div><br></div>Hi Amit,<br><br>Thanks =
for the quick fixes.</div><div class=3D"gmail_quote gmail_quote_container">=
<br></div><div class=3D"gmail_quote gmail_quote_container">I checked v1-000=
1 + v1-0002 against current master (e18b0cb7) with an assertion/debug build=
.</div><div class=3D"gmail_quote gmail_quote_container"><br></div><div clas=
s=3D"gmail_quote gmail_quote_container">- Both apply cleanly to master (in =
sequence)<br>- Defect 1 same-FK re-entry no longer crashes; the original sh=
ape completes and leaves the expected rows<br>- Defect 2 subtransaction-abo=
rt case now raises the FK violation instead of committing orphans<br>- For =
your defect 3 question: with 0002 applied, my reproducer no longer commits =
the child2 orphan. It raises:<br>=C2=A0 =C2=A0 ERROR: insert or update on t=
able &quot;child2&quot; violates foreign key constraint &quot;child2_fkey&q=
uot;<br>=C2=A0 =C2=A0 DETAIL: Key (a)=3D(999999) is not present in table &q=
uot;parent&quot;.<br><br>After the error, child2_orphans =3D 0 and child2 i=
s empty in my run.<br><br>I also ran the regression suite in that tree; for=
eign_key passed, and the full run reported all 245 tests passed.<br><br>So =
v1 looks good to me for the three reported cases.<br><br>Thanks!</div><div =
class=3D"gmail_quote gmail_quote_container"><br>Nik </div></div>

--00000000000040c6190653e1dc64--