Re: Eliminating SPI / SQL from some RI triggers - take 3

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.

--
Thanks, Amit Langote