MIME-Version: 1.0
References: 
 <CA+HiwqF4C0ws3cO+z5cLkPuvwnAwkSp7sfvgGj3yQ=Li6KNMqA@mail.gmail.com>
 <CA+HiwqGM6nvAV5O+=Nr+BXMPWOma0oeCRVzVP0XiLE8zX5TVAg@mail.gmail.com>
 <CA+HiwqGMaovCUgDbGxVGnK0Mrivr+ph3YE2Ws+47-ugyPb4f7g@mail.gmail.com>
 <CAFj8pRDaiBe_GOLk_yyYHTtPiDAAaLOM8u1-=Q3ZgXBTH+1igg@mail.gmail.com>
 <CA+HiwqGA5Ay_MR0eJEEbt4j6WrVh4F+AasTp8yCbs5aJLOJn6Q@mail.gmail.com>
 <CAEG8a3JM=NoqiTK0V6S9FNxZPvy1+C5F7rfafTtPKBVWnunL-g@mail.gmail.com>
 <CA+HiwqEyiLCY6MTLbOJXDdLNNQLaURYHvdW797MQgbjEK9od4Q@mail.gmail.com>
 <CAEG8a3+VBpwPf1Rm-ECD90whM9b3YnGhux5CVXdsL6khiBfzRQ@mail.gmail.com>
 <CA+HiwqF2UHzF0sKCp-F2a-U29rqh_9ZPy=f1h+Fh_=M8efj3pg@mail.gmail.com>
 <CAEG8a3L9Ew-WL8sxLROVOcypeaENPmd8qCmMvz4geoGL1TDGCA@mail.gmail.com>
 <CAEG8a3+nUFQo4sdPQF9xy0J73J8RFJ5U9A5+_kMosGDaZ+1sXA@mail.gmail.com>
 <CA+HiwqFB9b-ab=L8V4Z0FPqG1HHD=gzi_-N=9HxO3Dy8Xn=GnA@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqFB9b-ab=L8V4Z0FPqG1HHD=gzi_-N=9HxO3Dy8Xn=GnA@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Fri, 20 Mar 2026 17:20:04 +0900
Message-ID: 
 <CA+HiwqHXStQJRKYP-ANVeJjuNkQQKW6QZXY7oeCCoO-9nF6f+w@mail.gmail.com>
Subject: Re: Eliminating SPI / SQL from some RI triggers - take 3
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Tomas Vondra <tomas@vondra.me>
Content-Type: multipart/alternative; boundary="000000000000dd8742064d705ab2"
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqHXStQJRKYP-ANVeJjuNkQQKW6QZXY7oeCCoO-9nF6f%2Bw%40mail.gmail.com>
Precedence: bulk

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

On Mon, Mar 16, 2026 at 23:03 Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Junwang,
>
> Thanks for sending the new version.
>
> On Tue, Mar 10, 2026 at ... Junwang Zhao <zhjwpku@gmail.com> wrote:
> > 1.
> > Move ri_ReportViolation into ri_FastPathCheck, so table_open is no
> > longer needed, and ri_FastPathCheck now returns void.
>
> Good, kept.
>
> > 2.
> > After adding the batch fast path, the original ri_FastPathCheck is only
> > used by the ALTER TABLE validation path. This path cannot use the
> > cache because the registered AfterTriggerBatch callback will never run.
> > Therefore, the use_cache branch can be removed.
>
> Agreed.  I went a step further and restructured 0002 to avoid the
> use_cache branching entirely.  Instead of adding if/else blocks to
> ri_FastPathCheck, 0002 now adds a separate ri_FastPathCheckCached()
> function with its own resource lifecycle.  0003 then replaces it with
> ri_FastPathBatchAdd() -- a clean swap rather than completely undoing
> what 0002 added.  This also removes the use_cache parameter from
> ri_FastPathProbeOne; the memory context switch to
> TopTransactionContext is now the caller's responsibility.
>
> > 3.
> > ri_FastPathBatchFlush creates a new fk_slot but does not cache it in
> > RI_FastPathEntry. I tried caching it in v5-0006 and ran some benchmarks,
> > it didn't show much improvement.
>
> I put the fk_slot in the cache entry since it's a small change.
>
> > 4.
> > ri_FastPathFlushArray currently uses SK_SEARCHARRAY only for
> > single-column checks. [...] my current understanding is that
> > SK_SEARCHARRAY may not work for multi-column checks.
>
> Right, I haven't investigated this deeply either.  The FlushLoop
> fallback is the right approach for now.  If we want to explore a
> SEARCHARRAY approach for multi-column keys in a follow-up, it would be
> worth checking with Peter Geoghegan or someone else familiar with the
> btree SAOP internals on how multiple array keys across columns
> are iterated and whether that's usable at all for this use case.
>
> Attached is v6, three patches -- combined the old 0003 (buffering) and
> 0004 (SK_SEARCHARRAY) into a single 0003, since the buffering alone
> has no performance benefit (or at least only minor) and the split
> added unnecessary diff/rebase churn.
>
> The biggest change in this version is the snapshot handling.  Looking
> more carefully at what the SPI path actually does for RI_FKey_check
> (non-partitioned PK, detectNewRows = false), I found that
> ri_PerformCheck passes InvalidSnapshot to SPI_execute_snapshot, and
> _SPI_execute_plan ends up doing
> PushActiveSnapshot(GetTransactionSnapshot()).  So the SPI path scans
> with the transaction snapshot, not the latest
> snapshot.
>
> So I've changed the fast path to match: ri_FastPathCheck now uses
> GetTransactionSnapshot() directly.  Under READ COMMITTED this is a
> fresh snapshot; under REPEATABLE READ it's the frozen
> transaction-start snapshot, so PK rows committed after the transaction
> started are simply not visible.  This means the second index probe
> (the IsolationUsesXactSnapshot crosscheck block) is no longer needed
> and is removed.  The existing fk-snapshot isolation test confirms this
> is the correct behavior.
>
> Other changes since v5:
>
> * Fixed the batch callback firing during nested SPI: another AFTER
> trigger doing DML via SPI would call AfterTriggerEndQuery at a nested
> level, tearing down our cache mid-batch.  Fixed by checking
> query_depth inside FireAfterTriggerBatchCallbacks.  Added a test case
> with a trigger that auto-provisions PK rows via SPI.
>
> * Security context is now restored before ri_ReportViolation, not
> after (the ereport doesn't return).
>
> * search_vals[] and matched[] moved from RI_FastPathEntry to stack
> locals in ri_FastPathFlushArray -- they're rewritten from scratch on
> every flush with no state carried between calls.
>
> * Various comment fixes.
>
> I think this is getting close to committable shape. That said, another
> pair of eyes would be reassuring before I pull the trigger. Tomas, if
> you've had a chance to look, would welcome your thoughts.


Adding Tomas to cc -- I thought he was already on the thread when I sent
this. Tomas, context is in the quoted email below. Junwang has posted a
couple of updated versions since then addressing review feedback from Haibo
Yan (memory context handling for the flush path, batch size rationale).
Latest patches are in the thread. If you have time, would appreciate a look.

Thanks, Amit

>

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

<div dir=3D"auto">On Mon, Mar 16, 2026 at 23:03 Amit Langote &lt;<a href=3D=
"mailto:amitlangote09@gmail.com">amitlangote09@gmail.com</a>&gt; wrote:<br>=
</div><div dir=3D"auto"><div class=3D"gmail_quote gmail_quote_container"><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb=
(204,204,204)" dir=3D"auto">Hi Junwang,<br>
<br>
Thanks for sending the new version.<br>
<br>
On Tue, Mar 10, 2026 at ... Junwang Zhao &lt;<a href=3D"mailto:zhjwpku@gmai=
l.com" target=3D"_blank">zhjwpku@gmail.com</a>&gt; wrote:<br>
&gt; 1.<br>
&gt; Move ri_ReportViolation into ri_FastPathCheck, so table_open is no<br>
&gt; longer needed, and ri_FastPathCheck now returns void.<br>
<br>
Good, kept.<br>
<br>
&gt; 2.<br>
&gt; After adding the batch fast path, the original ri_FastPathCheck is onl=
y<br>
&gt; used by the ALTER TABLE validation path. This path cannot use the<br>
&gt; cache because the registered AfterTriggerBatch callback will never run=
.<br>
&gt; Therefore, the use_cache branch can be removed.<br>
<br>
Agreed.=C2=A0 I went a step further and restructured 0002 to avoid the<br>
use_cache branching entirely.=C2=A0 Instead of adding if/else blocks to<br>
ri_FastPathCheck, 0002 now adds a separate ri_FastPathCheckCached()<br>
function with its own resource lifecycle.=C2=A0 0003 then replaces it with<=
br>
ri_FastPathBatchAdd() -- a clean swap rather than completely undoing<br>
what 0002 added.=C2=A0 This also removes the use_cache parameter from<br>
ri_FastPathProbeOne; the memory context switch to<br>
TopTransactionContext is now the caller&#39;s responsibility.<br>
<br>
&gt; 3.<br>
&gt; ri_FastPathBatchFlush creates a new fk_slot but does not cache it in<b=
r>
&gt; RI_FastPathEntry. I tried caching it in v5-0006 and ran some benchmark=
s,<br>
&gt; it didn&#39;t show much improvement.<br>
<br>
I put the fk_slot in the cache entry since it&#39;s a small change.<br>
<br>
&gt; 4.<br>
&gt; ri_FastPathFlushArray currently uses SK_SEARCHARRAY only for<br>
&gt; single-column checks. [...] my current understanding is that<br>
&gt; SK_SEARCHARRAY may not work for multi-column checks.<br>
<br>
Right, I haven&#39;t investigated this deeply either.=C2=A0 The FlushLoop<b=
r>
fallback is the right approach for now.=C2=A0 If we want to explore a<br>
SEARCHARRAY approach for multi-column keys in a follow-up, it would be<br>
worth checking with Peter Geoghegan or someone else familiar with the<br>
btree SAOP internals on how multiple array keys across columns<br>
are iterated and whether that&#39;s usable at all for this use case.<br>
<br>
Attached is v6, three patches -- combined the old 0003 (buffering) and<br>
0004 (SK_SEARCHARRAY) into a single 0003, since the buffering alone<br>
has no performance benefit (or at least only minor) and the split<br>
added unnecessary diff/rebase churn.<br>
<br>
The biggest change in this version is the snapshot handling.=C2=A0 Looking<=
br>
more carefully at what the SPI path actually does for RI_FKey_check<br>
(non-partitioned PK, detectNewRows =3D false), I found that<br>
ri_PerformCheck passes InvalidSnapshot to SPI_execute_snapshot, and<br>
_SPI_execute_plan ends up doing<br>
PushActiveSnapshot(GetTransactionSnapshot()).=C2=A0 So the SPI path scans<b=
r>
with the transaction snapshot, not the latest<br>
snapshot.<br>
<br>
So I&#39;ve changed the fast path to match: ri_FastPathCheck now uses<br>
GetTransactionSnapshot() directly.=C2=A0 Under READ COMMITTED this is a<br>
fresh snapshot; under REPEATABLE READ it&#39;s the frozen<br>
transaction-start snapshot, so PK rows committed after the transaction<br>
started are simply not visible.=C2=A0 This means the second index probe<br>
(the IsolationUsesXactSnapshot crosscheck block) is no longer needed<br>
and is removed.=C2=A0 The existing fk-snapshot isolation test confirms this=
<br>
is the correct behavior.<br>
<br>
Other changes since v5:<br>
<br>
* Fixed the batch callback firing during nested SPI: another AFTER<br>
trigger doing DML via SPI would call AfterTriggerEndQuery at a nested<br>
level, tearing down our cache mid-batch.=C2=A0 Fixed by checking<br>
query_depth inside FireAfterTriggerBatchCallbacks.=C2=A0 Added a test case<=
br>
with a trigger that auto-provisions PK rows via SPI.<br>
<br>
* Security context is now restored before ri_ReportViolation, not<br>
after (the ereport doesn&#39;t return).<br>
<br>
* search_vals[] and matched[] moved from RI_FastPathEntry to stack<br>
locals in ri_FastPathFlushArray -- they&#39;re rewritten from scratch on<br=
>
every flush with no state carried between calls.<br>
<br>
* Various comment fixes.<br>
<br>
I think this is getting close to committable shape. That said, another<br>
pair of eyes would be reassuring before I pull the trigger. Tomas, if<br>
you&#39;ve had a chance to look, would welcome your thoughts.</blockquote><=
div dir=3D"auto"><br></div><div dir=3D"auto"><div style=3D"font-size:inheri=
t" dir=3D"auto"><div style=3D"font-size:inherit">Adding Tomas to cc -- I th=
ought he was already on the thread when I sent this. Tomas, context is in t=
he quoted email below. Junwang has posted a couple of updated versions sinc=
e then addressing review feedback from Haibo Yan (memory context handling f=
or the flush path, batch size rationale). Latest patches are in the thread.=
 If you have time, would appreciate a look.<br style=3D"font-size:inherit">=
</div><div style=3D"font-size:inherit" dir=3D"auto"><br></div><div style=3D=
"font-size:inherit" dir=3D"auto">Thanks, Amit=C2=A0</div></div></div><block=
quote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-w=
idth:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204=
,204,204)" dir=3D"auto"></blockquote></div></div>

--000000000000dd8742064d705ab2--