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

From: Amit Langote <[email-masked]>
Date: Tue, 21 Oct 2025 13:07:19 +0900

On Thu, Apr 3, 2025 at 7:19 PM Amit Langote <[email-masked]> wrote:
> On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <[email-masked]> wrote:
> > We discussed $subject at [1] and [2] and I'd like to continue that
> > work with the hope to commit some part of it for v18.
>
> I did not get a chance to do any further work on this in this cycle,
> but plan to start working on it after beta release, so moving this to
> the next CF.  I will post a rebased patch after the freeze to keep the
> bots green for now.

Sorry for the inactivity. I've moved the patch entry in the CF app to
PG19-Drafts, since I don't plan to work on it myself in the immediate
future. However, Junwang Zhao has expressed interest in taking this
work forward, and I look forward to working with him on it.

-- 
Thanks, Amit Langote

---

From: Pavel Stehule <[email-masked]>
Date: Tue, 21 Oct 2025 07:10:10 +0200

Hi

> work forward, and I look forward to working with him on it.

This is very interesting and important feature - I can help with testing
and review if it will be necessary

Regards

Pavel

---

From: Amit Langote <[email-masked]>
Date: Wed, 22 Oct 2025 22:55:53 +0900

.
On Tue, Oct 21, 2025 at 2:10 PM Pavel Stehule <[email-masked]> wrote:
> út 21. 10. 2025 v 6:07 odesílatel Amit Langote <[email-masked]> napsal:
>>
> This is very interesting and important feature - I can help with testing and review if it will be necessary

Thanks for the interest.

Just to add a quick note on the current direction I’ve been discussing
off-list with Junwang:

The next iteration of this work will likely follow a hybrid "fast-path
+ fallback" design rather than the original pure fast-path approach.
The idea is to keep the optimization for straightforward cases where
the foreign key and referenced key can be verified by a direct index
probe, while falling back to the existing SPI path only when the
runtime behavior of the executor is non-trivial to replicate -- such
as visibility rechecks under concurrent updates -- or when the
constraint itself involves richer semantics, like temporal foreign
keys that require range and aggregation logic. That keeps the
optimization safe without changing the meaning of constraint
enforcement.

This direction comes partly in response to the feedback from Robert
and Tom in the earlier Eliminating SPI threads, who raised concerns
that a fast path might silently diverge from what the executor does at
runtime in subtle cases. The fallback design aims to address that
directly: it keeps the optimization where it’s clearly safe, but
defers to the existing SPI-based implementation whenever correctness
might depend on executor behavior that would otherwise be difficult or
risky to reproduce locally.

In practice, this means adding a guarded fast path that performs the
index probe and tuple lock directly under the same snapshot and
security context that SPI would use, while caching stable metadata
such as index descriptors, scan keys, and operator information per
constraint or per statement. The fallback to SPI remains for the few
cases that either depend on executor behavior or need features beyond
a simple index probe:

* Concurrent updates or deletes: If table_tuple_lock() reports that
the target tuple was updated or deleted, we delegate to the SPI path
so that EvalPlanQual and visibility rules are applied as today.

* Partitioned parents: Skipped in v1 for simplicity, since they
require routing the probe through the correct partition using
PartitionDirectory. This can be added later as a separate patch once
the core mechanism is stable.

* Temporal foreign keys: These use range overlap and containment
semantics (&&, <@, range_agg()) that inherently involve aggregation
and multiple-row reasoning, so they stay on the SPI path.

Everything else -- multi-column keys, cross-type equality supported by
the index opfamily, collation matching, and RLS/ACL enforcement --
will be handled directly in the fast path.  The security behavior will
mirror the existing SPI path by temporarily switching to the parent
table's owner with SECURITY_LOCAL_USERID_CHANGE | SECURITY_NOFORCE_RLS
around the probe, like ri_PerformCheck() does.

For concurrency, the fast path locks the located parent tuple with
LockTupleKeyShare under GetActiveSnapshot(). If that succeeds (TM_Ok),
the check passes immediately. While non-TM_Ok cases fall back for now,
a later refinement could follow the update chain with
table_tuple_fetch_row_version() under the current snapshot and re-lock
the visible version, making the fast path fully self-contained.

That’s the direction Junwang and I plan to explore next.

--
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 1 Dec 2025 14:09:00 +0800

Hi,

> That’s the direction Junwang and I plan to explore next.

As Amit has already stated, we are approaching a hybrid "fast-path + fallback"
design.

0001 adds a fast path optimization for foreign key constraint checks
that bypasses the SPI executor, the fast path applies when the referenced
table is not partitioned, and the constraint does not involve temporal
semantics.

With the following test:

create table pk (a numeric primary key);
create table fk (a bigint references pk);
insert into pk select generate_series(1, 2000000);

head:

[local] zhjwpku@postgres:5432-90419=# insert into fk select
generate_series(1, 2000000, 2);
INSERT 0 1000000
Time: 13516.177 ms (00:13.516)

[local] zhjwpku@postgres:5432-90419=# update fk set a = a + 1;
UPDATE 1000000
Time: 15057.638 ms (00:15.058)

patched:

[local] zhjwpku@postgres:5432-98673=# insert into fk select
generate_series(1, 2000000, 2);
INSERT 0 1000000
Time: 8248.777 ms (00:08.249)

[local] zhjwpku@postgres:5432-98673=# update fk set a = a + 1;
UPDATE 1000000
Time: 10117.002 ms (00:10.117)

0002 cache fast-path metadata used by the index probe, at the current
time only comparison operator hash entries, operator function OIDs
and strategy numbers and subtypes for index scans. But this cache
doesn't buy any performance improvement.

Caching additional metadata should improve performance for foreign key checks.

Amit suggested introducing a mechanism for ri_triggers.c to register a
cleanup callback in the EState, which AfterTriggerEndQuery() could then
invoke to release per-statement cached metadata (such as the IndexScanDesc).
However, I haven't been able to implement this mechanism yet.

Amit and I agree that we can post the patches here for review now. We are
continuing to work on improving the metadata cache implementation.

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 23 Feb 2026 21:45:39 +0800

Hi Amit,

On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <[email-masked]> wrote:
>
> Hi Junwang,
>
> On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <[email-masked]> wrote:
> Thanks for working on this.  I've taken your patches as a starting
> point and reworked the series into two patches (attached): 1st is your
> 0001+0002 as the core patch that adds a gated fast-path alternative to
> SPI and 2nd where I added per-statement resource caching.  Doing the
> latter turned out to be not so hard thanks to the structure you chose
> to build the core fast path.  Good call on adding the RLS and ACL test
> cases, btw.
>
> So, 0001 is a functionally complete fast path: concurrency handling,
> REPEATABLE READ crosscheck, cross-type operators, security context,
> and metadata caching. 0002 implements the per-statement resource
> caching we discussed, though instead of sharing the EState between
> trigger.c and ri_triggers.c it uses a new AfterTriggerBatchCallback
> mechanism that fires at the end of each trigger-firing cycle
> (per-statement for immediate constraints, or until COMMIT for deferred
> ones). It layers resource caching on top so that the PK relation,
> index, scan descriptor, and snapshot stay open across all FK trigger
> invocations within a single trigger-firing cycle rather than being
> opened and closed per row.
>
> Note that phe previous 0002 (metadata caching) is folded into 0001,
> and most of the new fast-path logic added in 0001 now lives in
> ri_FastPathCheck() rather than inline in RI_FKey_check(), so the
> RI_FKey_check diff is just the gating call and SPI fallback.
>
> I re-ran the benchmarks (same test as yours, different machine):
>
> insert into fk select generate_series(1, 2000000, 2);
>
> master: 2444 ms  (median of 3 runs)
> 0001: 1382 ms  (43% faster)
> 0001+0002: 1202 ms  (51% faster, 13% over 0001 alone)

I can get similar improvement on my old mac intel chip:

master: 12963.993 ms
0001: 6641.692 ms, 48.8% faster
0001+0002: 5771.703 ms, 55.5% faster

>
> Also, with int PK / int FK (1M rows):
>
> create table pk (a int primary key);
> create table fk (a int references pk);
> insert into pk select generate_series(1, 1000000);
> insert into fk select generate_series(1, 1000000);
>
> master: 1000 ms
> 0001: 520 ms  (48% faster)
> 0001+0002: 432 ms  (57% faster, 17% over 0001 alone)

master: 11134.583 ms
0001: 5240.298 ms, 52.9% faster
0001+0002: 4554.215 ms, 59.1% faster

>
> The incremental gain from 0002 comes from eliminating per-row relation
> open/close, scan begin/end, slot alloc/free, and replacing per-row
> GetSnapshotData() with only curcid adjustment on the registered
> snapshot copy in the cache.
>
> The two current limitations are partitioned referenced tables and
> temporal foreign keys. Partitioned PKs are relatively uncommon in
> practice, so the non-partitioned case should cover most FK workloads,
> so I'm not sure it's worth the added complexity to support them.
> Temporal FKs are inherently multi-row, so they're a poor fit for a
> single-probe fast path.
>
> David Rowley mentioned off-list that it might be worth batching
> multiple FK values into a single index probe, leveraging the
> ScalarArrayOp btree improvements from PostgreSQL 17. The idea would be
> to buffer FK values across trigger invocations in the per-constraint
> cache (0002 already has the right structure for this), build a
> SK_SEARCHARRAY scan key, and let the btree AM walk the matching leaf
> pages in one sorted traversal instead of one tree descent per row. The
> locking and recheck would still be per-tuple, but the index traversal
> cost drops significantly. Single-column FKs are the obvious starting
> point. That seems worth exploring but can be done as a separate patch
> on top of this.

I will take a look at this in the following weeks.

>
> I think the series is in reasonable shape but would appreciate extra
> eyeballs, especially on the concurrency handling in ri_LockPKTuple()
> in 0001 and the snapshot lifecycle in 0002. Or anything else that
> catches one's eye.
>
> --
> Thanks, Amit Langote

I don't have any additional comments on the patch except one minor nit,
maybe merge the following two if conditions into one, not a strong opinion
though.

if (use_cache)
{
/*
* The snapshot was registered once when the cache entry was created.
* We just patch curcid to reflect the new command counter.
* SnapshotSetCommandId() only patches process-global statics, not
* registered copies, so we do it directly.
*
* The xmin/xmax/xip fields don't need refreshing: within a single
* statement batch, only curcid changes between rows.
*/
Assert(fpentry && fpentry->snapshot != NULL);
snapshot = fpentry->snapshot;
snapshot->curcid = GetCurrentCommandId(false);
}
else
snapshot = RegisterSnapshot(GetLatestSnapshot());

if (use_cache)
{
pk_rel = fpentry->pk_rel;
idx_rel = fpentry->idx_rel;
scandesc = fpentry->scandesc;
slot = fpentry->slot;
}
else
{
pk_rel = table_open(riinfo->pk_relid, RowShareLock);
idx_rel = index_open(riinfo->conindid, AccessShareLock);
scandesc = index_beginscan(pk_rel, idx_rel,
snapshot, NULL,
riinfo->nkeys, 0);
slot = table_slot_create(pk_rel, NULL);
}

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Sat, 28 Feb 2026 16:08:05 +0900

Hi Junwang,

> 0001+0002: 4554.215 ms, 59.1% faster

Thanks for testing, good to see similar numbers.  I had forgotten to
note that these results are when these PK index probes don't do any
I/O, though you might be aware of that. Below, I report some numbers
that Tomas Vondra shared with me off-list where the probes do have to
perform I/O and there the benefits from only this patch set are only
marginal.

> slot = table_slot_create(pk_rel, NULL);

Good idea, done.

While polishing 0002, I revisited the snapshot caching semantics. The
previous commit message hand-waved about only curcid changing between
rows, but GetLatestSnapshot() also reflects other backends' commits,
so reusing the snapshot is a deliberate semantic change from the SPI
path. I think it's safe because curcid is all we need for
intra-statement visibility, concurrent commits either already happened
before our snapshot (and are visible) or are racing with our statement
and wouldn't be seen reliably even with per-row snapshots since the
order in which FK rows are checked is nondeterministic, and
LockTupleKeyShare prevents the PK row from disappearing regardless. In
essence, we're treating all the FK checks within a trigger-firing
cycle as a single plan execution that happens to scan N rows, rather
than N independent SPI queries each taking a fresh snapshot. That's
the natural model -- a normal SELECT ... FOR KEY SHARE plan doesn't
re-take GetLatestSnapshot() between rows either.

Similarly, the permission check (schema USAGE + table SELECT) is now
done once at cache entry creation in ri_FastPathGetEntry() rather than
on every flush. The RI check runs as the PK table owner, so we're
verifying that the owner can access their own table -- a condition
that won't change unless someone explicitly revokes from the owner,
which would also break the SPI path.

> I will take a look at this in the following weeks.

I ended up going ahead with the batching and SAOP idea that David
mentioned -- I had a proof-of-concept working shortly after posting v3
and kept iterating on it.  So attached set is now:

0001 - Core fast path (your 0001+0002 reworked, as before)

0002 - Per-batch resource caching (PK relation, index, scandesc, snapshot)

0003 - FK row buffering: materialize FK tuples into a per-constraint
batch buffer (64 rows), flush when full or at batch end

0004 - SK_SEARCHARRAY for single-column FKs: build an array from the
buffered FK values and do one index scan instead of 64 separate tree
descents.  Multi-column FKs fall back to a per-row loop.

0003 is pure infrastructure -- it doesn't improve performance on its
own because the per-row index descent still dominates.  The payoff
comes in 0004.

Numbers (same machine as before, median of 3 runs):

numeric PK / bigint FK, 1M rows:
master: 2487 ms
0001..0004: 1168 ms  (2.1x)

int PK / int FK, 500K rows:
master: 1043 ms
0001..0004: 335 ms  (3.1x)

The int/int case benefits most because the per-row cost is lower, so
the SAOP traversal savings are a larger fraction of the total. The
numeric/bigint case still sees a solid improvement despite the
cross-type cast overhead.

Tomas Vondra also tested with an I/O-intensive workload (dataset
larger than shared_buffers, combined with his and Peter Geoghegan's
I/O prefetching patches) and confirmed that the batching + SAOP
approach helps there too, not just in the CPU-bound / memory-resident
case.  In fact he showed that the patches here don't make a big dent
when the main bottleneck is I/O as shown in numbers that he shared in
an off-list email:

master: 161617 ms
ri-check (0001..0004): 149446 ms  (1.08x)
ri-check + i/o prefetching: 50885 ms  (3.2x)

So the RI patches alone only give ~8% here since most time is waiting
on reads.  But the batching gives the prefetch machinery a window of
upcoming probes to issue readahead against, so the two together yield
3.2x.

Tomas also caught a memory context bug in the batch flush path: the
cached scandesc lives in TopTransactionContext, but the btree AM
defers _bt_preprocess_keys allocation to the first getnext call, which
pallocs into CurrentMemoryContext. If that's a short-lived
per-trigger-row context, the scandesc has dangling pointers on the
next rescan. Fixed by switching to TopTransactionContext before the
probe loop.

Finally, I've fixed a number of other small and not-so-small bugs
found while polishing the old patches and made other stylistic
improvements. One notable change is that I introduced a FastPathMeta
struct to store the fast path metadata instead of dumping those arrays
in the RI_ConstraintInfo. It's allocated lazily on first use and holds
the per-key compare entries, operator procedures, and index strategy
info needed by the scan key construction, so RI_ConstraintInfo doesn't
pay for them when the fast path isn't used.

-- 
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 1 Mar 2026 13:22:49 +0100

On 2/28/26 08:08, Amit Langote wrote:
> ...

I tested this (with the index prefetching v11 patch), because I wanted
to check if the revised API works fine for other use cases, not just the
regular index scans. Turns out the answer is "yes", the necessary tweaks
to the FK batching patch were pretty minimal, and at the same time it
did help quite a bit for cases bottle-necked on I/O.

FWIW I wonder how difficult would it be to do something like this for
inserts into indexes. It's an orthogonal issue to FK checks (especially
for the CPU-bound cases this thread focuses on), but it's a bit similar
to the I/O-bound case. In fact, I now realize I actually did a PoC for
that in 2023-11 [1], but it went stale ...

benchmarks
----------

Anyway, thinking about the CPU-bound case, I decided to do a bit of
testing on my own. I was wondering about three things:

(a) how does the improvement depend on data distribution
(b) could it cause regressions for small inserts
(c) how sensitive is the batch size

So I devised two simple benchmarks:

1) run-pattern.sh - Inserts batches of values into a table, both the
batch and table can be either random or sequential. It's either 100k or
1M rows, logged or unlogged, etc.

2) run-pgbench.sh - Runs short pgbench inserting data into a table,
similar to (1), but with very few rows - so the timing approach is not
suitable to measure this.

Both scripts run against master, and then patched branch with three
batch sizes (default 64, 16 and 256).

results
-------

The results are very positive - see the attached PDF files comparing the
patched builds to master.

I have not found a single case where the batching causes regressions.
This surprised me a bit, I've expected small regressions for single-row
inserts in the pgbench test, but even that shows a small (~5%) gain.
Even just 2-row inserts show +25% improvement in pgbench throughput.

There are a couple cases where it matches master, I assume that's for
I/O bound cases where the CPU optimizations do not really matter. That's
expected, of course.

I don't see much sensitivity on the batch size. The 256 batches seem to
be a bit slower, but there's little difference between 16 and 64. So I'd
say 64 seems reasonable.

Overall, I think these results looks quite good. I haven't looked at the
code very closely, not beyond adjusting it to work with index prefetch.

[1] https://commitfest.postgresql.org/patch/4622/

-- 
Tomas Vondra

---

From: Amit Langote <[email-masked]>
Date: Mon, 2 Mar 2026 16:49:05 +0900

Hi Tomas,

Thanks for the thorough benchmarking.

> did help quite a bit for cases bottle-necked on I/O.

Do you think those changes to the FK batching are only necessary for
making it work with your patch or is that worth including with the set
here because it's generally applicable?

> that in 2023-11 [1], but it went stale ...

Interesting. I hadn't seen your earlier PoC. Does the current I/O
prefetching infrastructure simplify that approach, or are they
independent paths? The old patch calls PrefetchBuffer() directly on
the leaf, which seems orthogonal to the scan-side prefetching. Either
way, would be nice to see more paths benefit from batching.

> benchmarks

This is reassuring. I too was half-expecting the batching
infrastructure to add measurable overhead for single-row inserts, but
it looks like the SPI bypass alone more than covers it.

> be a bit slower, but there's little difference between 16 and 64. So I'd

Agreed. Interesting that 16 is consistently a little better than 64 in
the patterns benchmark. I'd guess that's the per-PK-index-match linear
scan over the batch cost showing up, since it's O(batch_size) per PK
match. 256 being noticeably worse fits that picture. 64 seems like a
good middle ground since the pgbench numbers show virtually no
difference between 16 and 64.

The best-case numbers are striking -- when both the PK table and the
FK values being inserted are in sequential order, the unlogged
patterns case hits 4-5x, wow. I guess that makes sense because
sequential FK values turn into a sorted SAOP array that walks
consecutive leaf pages, so it's essentially a single sequential scan
of the relevant index portion.

> code very closely, not beyond adjusting it to work with index prefetch.

If you get a chance, I'd welcome a closer look. Your memory context
catch was a real bug that I'd missed entirely. The area that would
benefit most from a second pair of eyes is the snapshot and permission
caching semantics in 0002.  The argument for why reusing the snapshot
and checking permissions once per batch is safe rather than per-row is
sound I think, but the effects are global and hard to validate by
testing alone..

-- 
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 2 Mar 2026 23:30:58 +0800

On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <[email-masked]> wrote:
> on every flush.

nice improvement.

> The RI check runs as the PK table owner, so we're

impressive!

> improvements. One notable change is that I introduced a FastPathMeta

Yeah, this is much better than the fpmeta_valid field.

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 10 Mar 2026 20:28:47 +0800

Hi,

> > > slot = table_slot_create(pk_rel, NULL);

I had an offline discussion with Amit today. There were a few small things
that could be improved, so I posted a new version of the patch set.

1.

+ if (ri_fastpath_is_applicable(riinfo))
+ {
+ bool found = ri_FastPathCheck(riinfo, fk_rel, newslot);
+
+ if (found)
+ return PointerGetDatum(NULL);
+
+ /*
+ * ri_FastPathCheck opens pk_rel internally; we need it for
+ * ri_ReportViolation.  Re-open briefly.
+ */
+ pk_rel = table_open(riinfo->pk_relid, RowShareLock);
+ ri_ReportViolation(riinfo, pk_rel, fk_rel,
+    newslot, NULL,
+    RI_PLAN_CHECK_LOOKUPPK, false, false);
+ }

Move ri_ReportViolation into ri_FastPathCheck, so table_open is no
longer needed, and ri_FastPathCheck now returns void. Since Amit
agreed this is the right approach, I included it directly in v5-0001.

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.

I made this change in v5-0004 and also updated some related comments.
Once we agree the changes are correct, it can be merged into v5-0003.

3.

+ fk_slot = MakeSingleTupleTableSlot(RelationGetDescr(fk_rel),
+    &TTSOpsHeapTuple);

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. This might be because the slot creation
function is called once per batch rather than once per row, so the overall
impact is minimal. I'm posting this here for Amit to take a look and decide
whether we should adopt it or drop it, since I mentioned the idea to
him earlier.

4.

ri_FastPathFlushArray currently uses SK_SEARCHARRAY only for
single-column checks. I asked whether this could be extended to support
multi-column cases, and Amit encouraged me to look into it.

After a brief investigation, it seems that ScanKeyEntryInitialize only allows
passing a single subtype/collation/procedure, which makes it difficult to
handle multiple types. Based on this, my current understanding is that
SK_SEARCHARRAY may not work for multi-column checks.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Mon, 16 Mar 2026 23:03:41 +0900

Hi Junwang,

Thanks for sending the new version.

On Tue, Mar 10, 2026 at ... Junwang Zhao <[email-masked]> wrote:
> longer needed, and ri_FastPathCheck now returns void.

Good, kept.

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

> it didn't show much improvement.

I put the fk_slot in the cache entry since it's a small change.

> single-column checks. [...] my current understanding is that

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

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 18 Mar 2026 23:34:30 +0800

Hi Haibo,

On Tue, Mar 17, 2026 at 8:28 AM Haibo Yan <[email-masked]> wrote:
>
> Hi, Amit and Junwang
>
> Thanks for the latest patch. I think the overall direction makes sense, and the single-column SK_SEARCHARRAY path looks like one of the most valuable optimizations here. The patch also seems to cover several important cases, including deferred constraints, duplicate FK values, and multi-column fallback behavior.
>
> After reading through the patch, I have one major comments and a few smaller ones.

Thanks for your review.

>
> 1. TopTransactionContext usage during batched flush may be too coarse-grained
> My biggest concern is the use of TopTransactionContext around the batched flush path.
> As written, ri_FastPathBatchFlush() switches to TopTransactionContext before calling ri_FastPathFlushArray() / ri_FastPathFlushLoop(). That seems broad enough that temporary allocations made during the flush may end up there.
> In particular, in ri_FastPathFlushArray(), I think the objects worth checking carefully are the pass-by-reference Datums returned by the per-element cast call and stored in search_vals[], e.g.
>
> ```search_vals[i] = FunctionCall3(&entry->cast_func_finfo, ...);```
> If those cast results are separately allocated in the current memory context, then pfree(arr) only frees the constructed array object itself; it does not obviously free those intermediate cast results. If so, those allocations could survive until end of transaction rather than just until the end of the current flush.
> Maybe this is harmless in practice, but I think it needs a closer look. It might be better to use a dedicated short-lived context for per-flush temporary allocations, reset it after each flush, or otherwise separate allocations that really need transaction lifetime from those that are only needed transiently during batched processing.

Yeah, that concern is reasonable. After a brief discussion with Amit,
we now replace the TopTransactionContext usage with two
purpose-specific contexts, scan_cxt for index AM allocations, freed
at teardown, and flush_cxt for per-flush transient work, reset each flush,
TopTransactionContext is parent of scan_cxt, and scan_cxt is parent
of flush_cxt, so MemoryContextDelete(scan_cxt) in teardown cleans up
both.

These changes are in v7-0004.

>
> 2. RI_FastPathEntry comment mentions the wrong function name
> The comment above RI_FastPathEntry says it contains resources needed by ri_FastPathFlushBatch(), but the function is named ri_FastPathBatchFlush().

Fixed.

>
> 3. RI_FASTPATH_BATCH_SIZE needs some rationale
> RI_FASTPATH_BATCH_SIZE = 64 may well be a reasonable compromise, but right now it reads like a magic number.
> This choice seems especially relevant because the patch has two opposing effects:
> 3-1. larger batches should amortize the array-scan work better,
> 3-2. but the matched[] bookkeeping in ri_FastPathFlushArray() is O(batch_size^2) in the worst case.
> So I think it would help to include at least a brief rationale in a comment or in the commit message.

Added.

>
> 4. Commit message says the entry stashes fk_relid, but the code actually stashes riinfo
> The commit message says the entry stashes fk_relid and can reopen the relation if needed. Unless I am misreading it, the code actually stores riinfo and later uses riinfo->fk_relid. The distinction is small, but I think the wording should match the implementation more closely.

I changed the commit message to:

Since the FK relation may already be closed by flush time (e.g. for
deferred constraints at COMMIT), reopens the relation using
entry->riinfo->fk_relid if needed.

>
> Thanks again for working on this.
>
> Best regards,
>
> Haibo Yan
>
>
> On Mar 10, 2026, at 5:28 AM, Junwang Zhao <[email-masked]> wrote:
>
> Hi,
>
> <v5-0005-Use-SK_SEARCHARRAY-for-batched-fast-path-FK-probe.patch><v5-0006-Reuse-FK-tuple-slot-across-fast-path-batches.patch><v5-0002-Cache-per-batch-resources-for-fast-path-foreign-k.patch><v5-0004-Refine-fast-path-FK-validation-path.patch><v5-0003-Buffer-FK-rows-for-batched-fast-path-probing.patch><v5-0001-Add-fast-path-for-foreign-key-constraint-checks.patch>
>
>

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 20 Mar 2026 00:19:53 +0800

> > in 0001 and the snapshot lifecycle in 0002. Or anything else that

I squashed 0004 into 0003 so that each file can be committed independently.
I also runned pgindent for each file.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Mar 2026 17:20:04 +0900

> 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

---

From: Amit Langote <[email-masked]>
Date: Tue, 24 Mar 2026 20:47:17 +0900

Hi Junwang,

> I also runned pgindent for each file.

Thanks for that.

Here's another version.

In 0001, I noticed that the condition change in ri_HashCompareOp could
be simplified further.  Also improved the commentary surrounding that.
I also updated the commit message to clarify parity with the SPI path.

Updated the commit message of 0002 to talk about why caching the
snapshot for the entire trigger firing cycle of a given constraint
makes a trade off compared to the SPI path which retakes the snapshot
for every row checked and could in principle avoid failure for FK rows
whose corresponding PK row was added by a concurrently committed
transaction, at least in the READ COMMITTED case.

Updated the commit message of 0003 to clarify that it replaces
ri_FastPathCheckCached() from 0002 with the BatchAdd/BatchFlush pair,
and that the cached resources are used unchanged -- only the probing
cadence changes from per-row to per-flush.  Per-flush CCI is safe
because all AFTER triggers for the buffered rows have already fired
by flush time; a new test case is added to show that.

Finally I added a short line at the end of each patch's commit message
to mention the speedup observed at each stage.  There are placeholders
such as <commit-hash-0001> that I will replace by an actual commit
hash before  pushing.

I will continue staring at these for any remaining issues before
pushing them one-by-one at some point by early next week.  Happy to
hear any thoughts before I push.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Tue, 24 Mar 2026 22:56:18 +0900

> by flush time; a new test case is added to show that.

Kept thinking about this on a walk after I sent this and came to the
conclusion that it might be better to just not cache the snapshot with
only the above argument in its favor.  If repeated GetSnapshotData()
is expensive, the solution should be to fix that instead of simply
side-stepping it.

By taking a snapshot per-batch without caching it, and so likewise the
IndexScanDesc, I'm seeing the same ~3x speedup in the batched
SK_SEARCHARRAY case, so I don't see much point in being very stubborn
about snapshot caching.  Like in the attached (there's an unrelated
memory context switch thinko fix).  Note that relations (pk_rel,
idx_rel) and the slot remain cached across the batch; only the
snapshot and scandesc are taken fresh per flush.

I'll post an updated version tomorrow morning.  I think it might be
better to just merge 0003 into 0002, because without snapshot and
scandesc caching the standalone value of 0002 is mostly just relation
and slot caching -- the interesting parts (batch callbacks, lifecycle
management) are all scaffolding for the batching.  So v10 will be two
patches: 0001 core fast path, 0002 everything else.

-- 
Thanks, Amit Langote
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 993c3ac49a3..f271ffccc00 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -224,10 +224,8 @@ typedef struct RI_FastPathEntry
 	Oid			conoid;			/* hash key: pg_constraint OID */
 	Relation	pk_rel;
 	Relation	idx_rel;
-	IndexScanDesc scandesc;
 	TupleTableSlot *pk_slot;
 	TupleTableSlot *fk_slot;
-	Snapshot	snapshot;		/* registered snapshot for the scan */
 	MemoryContext scan_cxt;		/* index scan allocations */
 	MemoryContext flush_cxt;	/* short-lived context for per-flush work */
 
@@ -301,9 +299,11 @@ static void ri_FastPathCheck(const RI_ConstraintInfo *riinfo,
 static void ri_FastPathBatchAdd(const RI_ConstraintInfo *riinfo,
 								Relation fk_rel, TupleTableSlot *newslot);
 static void ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
-								  const RI_ConstraintInfo *riinfo, Relation fk_rel);
+								  const RI_ConstraintInfo *riinfo, Relation fk_rel,
+								  Snapshot snapshot);
 static void ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
-								 const RI_ConstraintInfo *riinfo, Relation fk_rel);
+								 const RI_ConstraintInfo *riinfo, Relation fk_rel,
+								 Snapshot snapshot);
 static void ri_FastPathBatchFlush(RI_FastPathEntry *fpentry,
 								  Relation fk_rel);
 static bool ri_FastPathProbeOne(Relation pk_rel, Relation idx_rel,
@@ -2857,8 +2857,8 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
 	const RI_ConstraintInfo *riinfo = fpentry->riinfo;
 	Relation	pk_rel = fpentry->pk_rel;
 	Relation	idx_rel = fpentry->idx_rel;
-	Snapshot	snapshot = fpentry->snapshot;
 	TupleTableSlot *fk_slot = fpentry->fk_slot;
+	Snapshot	snapshot;
 	Oid			saved_userid;
 	int			saved_sec_context;
 	MemoryContext oldcxt = CurrentMemoryContext;
@@ -2882,7 +2882,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
 	 * context per row whereas we do it once around the whole batch.
 	 */
 	CommandCounterIncrement();
-	snapshot->curcid = GetCurrentCommandId(false);
+	snapshot = RegisterSnapshot(GetTransactionSnapshot());
 
 	GetUserIdAndSecContext(&saved_userid, &saved_sec_context);
 	SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
@@ -2891,11 +2891,12 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
 						   SECURITY_NOFORCE_RLS);
 
 	if (riinfo->nkeys == 1)
-		ri_FastPathFlushArray(fpentry, fk_slot, riinfo, fk_rel);
+		ri_FastPathFlushArray(fpentry, fk_slot, riinfo, fk_rel, snapshot);
 	else
-		ri_FastPathFlushLoop(fpentry, fk_slot, riinfo, fk_rel);
+		ri_FastPathFlushLoop(fpentry, fk_slot, riinfo, fk_rel, snapshot);
 	MemoryContextSwitchTo(oldcxt);
 	SetUserIdAndSecContext(saved_userid, saved_sec_context);
+	UnregisterSnapshot(snapshot);
 
 	/* Free materialized tuples and reset */
 	for (int i = 0; i < fpentry->batch_count; i++)
@@ -2912,29 +2913,30 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
  */
 static void
 ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
-					 const RI_ConstraintInfo *riinfo, Relation fk_rel)
+					 const RI_ConstraintInfo *riinfo, Relation fk_rel,
+					 Snapshot snapshot)
 {
 	Relation	pk_rel = fpentry->pk_rel;
 	Relation	idx_rel = fpentry->idx_rel;
-	IndexScanDesc scandesc = fpentry->scandesc;
 	TupleTableSlot *pk_slot = fpentry->pk_slot;
-	Snapshot	snapshot = fpentry->snapshot;
+	IndexScanDesc scandesc;
 	Datum		pk_vals[INDEX_MAX_KEYS];
 	char		pk_nulls[INDEX_MAX_KEYS];
 	ScanKeyData skey[INDEX_MAX_KEYS];
+	bool		found = true;
+	/*
+	 * build_index_scankeys() may palloc cast results for cross-type FKs.
+	 * Use the entry's short-lived flush context so these don't accumulate
+	 * across batches.
+	 */
+	MemoryContext oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
 
+	scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
+							   riinfo->nkeys, 0);
 	for (int i = 0; i < fpentry->batch_count; i++)
 	{
-		bool		found = false;
 
 		ExecStoreHeapTuple(fpentry->batch[i], fk_slot, false);
-
-		/*
-		 * build_index_scankeys() may palloc cast results for cross-type FKs.
-		 * Use the entry's short-lived flush context so these don't accumulate
-		 * across batches.
-		 */
-		MemoryContextSwitchTo(fpentry->flush_cxt);
 		ri_ExtractValues(fk_rel, fk_slot, riinfo, false, pk_vals, pk_nulls);
 		build_index_scankeys(riinfo, idx_rel, pk_vals, pk_nulls, skey);
 		MemoryContextSwitchTo(fpentry->scan_cxt);
@@ -2943,11 +2945,15 @@ ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
 									snapshot, riinfo, skey, riinfo->nkeys);
 
 		if (!found)
-			ri_ReportViolation(riinfo, pk_rel, fk_rel,
-							   fk_slot, NULL,
-							   RI_PLAN_CHECK_LOOKUPPK, false, false);
+			break;
 	}
+	index_endscan(scandesc);
 	MemoryContextReset(fpentry->flush_cxt);
+	MemoryContextSwitchTo(oldcxt);
+	if (!found)
+		ri_ReportViolation(riinfo, pk_rel, fk_rel,
+						   fk_slot, NULL,
+						   RI_PLAN_CHECK_LOOKUPPK, false, false);
 }
 
 /*
@@ -2962,14 +2968,14 @@ ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
  */
 static void
 ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
-					  const RI_ConstraintInfo *riinfo, Relation fk_rel)
+					  const RI_ConstraintInfo *riinfo, Relation fk_rel,
+					  Snapshot snapshot)
 {
 	FastPathMeta *fpmeta = riinfo->fpmeta;
 	Relation	pk_rel = fpentry->pk_rel;
 	Relation	idx_rel = fpentry->idx_rel;
-	IndexScanDesc scandesc = fpentry->scandesc;
 	TupleTableSlot *pk_slot = fpentry->pk_slot;
-	Snapshot	snapshot = fpentry->snapshot;
+	IndexScanDesc scandesc;
 	Datum		search_vals[RI_FASTPATH_BATCH_SIZE];
 	bool		matched[RI_FASTPATH_BATCH_SIZE];
 	int			nvals = fpentry->batch_count;
@@ -2983,16 +2989,19 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
 	char		elem_align;
 	ArrayType  *arr;
 
-	Assert(fpmeta);
-
-	memset(matched, 0, nvals * sizeof(bool));
-
 	/*
 	 * Transient per-flush allocations (cast results, the search array) must
 	 * not accumulate across repeated flushes.  Use the entry's short-lived
 	 * flush context, reset after each flush.
 	 */
-	MemoryContextSwitchTo(fpentry->flush_cxt);
+	MemoryContext oldcxt = MemoryContextSwitchTo(fpentry->flush_cxt);
+
+	Assert(fpmeta);
+
+	memset(matched, 0, nvals * sizeof(bool));
+
+	scandesc = index_beginscan(pk_rel, idx_rel, snapshot, NULL,
+							   riinfo->nkeys, 0);
 
 	/*
 	 * Extract FK values, casting to the operator's expected input type if
@@ -3103,6 +3112,11 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
 		}
 	}
 
+	index_endscan(scandesc);
+
+	MemoryContextReset(fpentry->flush_cxt);
+	MemoryContextSwitchTo(oldcxt);
+
 	/* Report first unmatched row */
 	for (int i = 0; i < nvals; i++)
 	{
@@ -3114,8 +3128,6 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
 							   RI_PLAN_CHECK_LOOKUPPK, false, false);
 		}
 	}
-
-	MemoryContextReset(fpentry->flush_cxt);
 }
 
 /*
@@ -4106,9 +4118,6 @@ ri_FastPathTeardown(void)
 	hash_seq_init(&status, ri_fastpath_cache);
 	while ((entry = hash_seq_search(&status)) != NULL)
 	{
-		/* Close both scans before closing idx_rel. */
-		if (entry->scandesc)
-			index_endscan(entry->scandesc);
 		if (entry->idx_rel)
 			index_close(entry->idx_rel, NoLock);
 		if (entry->pk_rel)
@@ -4117,8 +4126,6 @@ ri_FastPathTeardown(void)
 			ExecDropSingleTupleTableSlot(entry->pk_slot);
 		if (entry->fk_slot)
 			ExecDropSingleTupleTableSlot(entry->fk_slot);
-		if (entry->snapshot)
-			UnregisterSnapshot(entry->snapshot);
 		if (entry->scan_cxt)
 			MemoryContextDelete(entry->scan_cxt);
 	}
@@ -4232,21 +4239,10 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel)
 		entry->pk_rel = table_open(riinfo->pk_relid, RowShareLock);
 		entry->idx_rel = index_open(riinfo->conindid, AccessShareLock);
 
-		/*
-		 * Register an initial snapshot.  Its curcid will be patched in place
-		 * on each subsequent row (see ri_FastPathBatchFlush()), avoiding
-		 * per-row GetSnapshotData() overhead.
-		 */
-		entry->snapshot = RegisterSnapshot(GetTransactionSnapshot());
-
 		entry->pk_slot = table_slot_create(entry->pk_rel, NULL);
 		entry->fk_slot = MakeSingleTupleTableSlot(RelationGetDescr(fk_rel),
 												  &TTSOpsHeapTuple);
 
-		entry->scandesc = index_beginscan(entry->pk_rel, entry->idx_rel,
-										  entry->snapshot, NULL,
-										  riinfo->nkeys, 0);
-
 		entry->scan_cxt = AllocSetContextCreate(TopTransactionContext,
 												"RI fast path scan context",
 												ALLOCSET_DEFAULT_SIZES);

---

From: Amit Langote <[email-masked]>
Date: Wed, 25 Mar 2026 09:41:11 +0900

> patches: 0001 core fast path, 0002 everything else.

And here's a set like that.  I noticed that we don't need a dedicated
scan_cxt now that scandesc is not cached and a few other
simplifications.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 30 Mar 2026 13:55:07 +0900

> scan_cxt now that scandesc is not cached and a few other

Junwang pointed out off-list that FK tuples added to
RI_FastPathEntry.batch[] were being copied into TopTransactionContext
rather than flush_cxt, so they would accumulate until the batch was
exhausted rather than being reclaimed per flush. Fixed in
ri_FastPathBatchAdd() in 0002.

Also added a couple of comments in trigger.c that were missing: an
Assert and explanation in RegisterAfterTriggerBatchCallback()
clarifying the query_depth >= 0 precondition, a comment at the
AfterTriggerEndQuery call site explaining why
FireAfterTriggerBatchCallbacks() must precede the query_depth
decrement and AfterTriggerFreeQuery, and brief intent comments at the
AfterTriggerFireDeferred and AfterTriggerSetState call sites.

Plan is to commit 0001 tomorrow barring objections and let it sit for
a bit before committing 0002. Feedback on 0002, particularly on the
AfterTriggerBatchCallback mechanism in trigger.c, welcome in the
meantime.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 30 Mar 2026 20:15:05 +0900

> AfterTriggerBatchCallback mechanism in trigger.c, welcome in the

Kept looking at 0002 and found a couple of things to improve or change
my thoughts about.  I decided to move the permission check from fast
path cache entry creation into ri_FastPathBatchFlush(), alongside the
snapshot, so that permission changes between flushes are respected
rather than checked once at batch start; the check happens for every
row in the SPI and non-batched fast path.  Also, improved comments in
a few places to mention design decisions better.

0001 is mostly unchanged from v11 except I updated its commit message
to explain why only RI_FKey_check is covered and not the action
triggers as the topic has come up in previous threads about this
topic.

Still planning to commit 0001 tomorrow.

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Tue, 31 Mar 2026 17:09:19 +0800

> On Mar 30, 2026, at 19:15, Amit Langote <[email-masked]> wrote:
> 
> <v12-0001-Add-fast-path-for-foreign-key-constraint-checks.patch><v12-0002-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch>

Hi Amit,

While reading the recent commits, I saw that 0001 has been pushed as 2da86c1ef9b5446e0e22c0b6a5846293e58d98e3. However, I also just noticed a use-after-free issue in ri_LoadConstraintInfo(). It dereferences conForm after ReleaseSysCache(tup), which is unsafe. I am attaching a tiny patch to fix that.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Amit Langote <[email-masked]>
Date: Tue, 31 Mar 2026 18:17:16 +0900

Hi,

> While reading the recent commits, I saw that 0001 has been pushed as 2da86c1ef9b5446e0e22c0b6a5846293e58d98e3. However, I also just noticed a use-after-free issue in ri_LoadConstraintInfo(). It der…

Thanks.  I noticed that too and pushed the fix an hour ago:

https://www.postgresql.org/message-id/E1w7U6V-002H6n-0o%40gemulon.postgresql.org

-- 
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 31 Mar 2026 18:57:46 +0800

> https://www.postgresql.org/message-id/E1w7U6V-002H6n-0o%40gemulon.postgresql.org

prion is happy now, the fix works, thanks.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Tue, 31 Mar 2026 20:26:35 +0900

On Tue, Mar 31, 2026 at 8:22 PM Daniel Gustafsson <[email-masked]> wrote:
> > On 31 Mar 2026, at 12:57, Junwang Zhao <[email-masked]> wrote:
>
> The widowbird failure seems to be SPI related as well, relevant portion of log
> below. Is that the same or another error?

prion was unhappy about something else, which I've fixed:
https://www.postgresql.org/message-id/E1w7U6V-002H6n-0o%40gemulon.postgresql.org

Though, I'm not sure if or why the fix is now the reason for
widowbird's failure.

> echo "# +++ tap install-check in src/test/modules/worker_spi +++" && rm -rf '/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi'/tmp_check && /usr/bin/mkdir -p '/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi'/tmp_check && cd . && TESTLOGDIR='/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/tmp_check/log' TESTDATADIR='/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/tmp_check' PATH="/mnt/data/buildfarm/buildroot/HEAD/inst/bin:/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi:$PATH" PGPORT='65678' top_builddir='/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/../../../..' PG_REGRESS='/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/../../../../src/test/regress/pg_regress' share_contrib_dir='/mnt/data/buildfarm/buildroot/HEAD/inst/share/postgresql/extension' /usr/bin/prove -I ../../../../src/test/perl/ -I . t/*.pl
> # +++ tap install-check in src/test/modules/worker_spi +++
> t/001_worker_spi.pl ........ ok
> # Tests were run but no plan was declared and done_testing() was not seen.
> # Looks like your test exited with 29 just after 8.
> t/002_worker_terminate.pl ..
> Dubious, test returned 29 (wstat 7424, 0x1d00)
> All 8 subtests passed
>
> Test Summary Report
> -------------------
> t/002_worker_terminate.pl (Wstat: 7424 (exited 29) Tests: 8 Failed: 0)
> Non-zero exit status: 29
> Parse errors: No plan found in TAP output
> Files=2, Tests=16, 28 wallclock secs ( 0.08 usr 0.01 sys + 6.63 cusr 2.93 csys = 9.65 CPU)
> Result: FAIL
> make[1]: *** [../../../../src/makefiles/pgxs.mk:439: installcheck] Error 1
> make[1]: Leaving directory '/mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi'
> make: *** [Makefile:87: installcheck-worker_spi-recurse] Error 2
> log files for step testmodules-install-check-en_GB.UTF-8:

Not sure what's going on here or how it's related to 68a8601ee.

-- 
Thanks, Amit Langote

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 31 Mar 2026 13:35:33 +0200

> On 31 Mar 2026, at 13:26, Amit Langote <[email-masked]> wrote:
> 

Not sure either, I just saw SPI when digging and wanted to check, but I agree
that it doesn't seem related.  More log excerpts:

[11:15:49.966](3.762s) ok 1 - dynamic bgworker 0 launched
[11:15:50.372](0.407s) ok 2 - background worker blocked the database creation
[11:15:50.413](0.041s) ok 3 - background worker is still running after CREATE DATABASE WITH TEMPLATE
[11:15:50.663](0.250s) ok 4 - dynamic bgworker 1 launched
[11:15:51.085](0.421s) ok 5 - dynamic bgworker stopped for CREATE DATABASE WITH TEMPLATE
[11:15:51.264](0.179s) ok 6 - dynamic bgworker 2 launched
[11:15:51.438](0.175s) ok 7 - dynamic bgworker stopped for ALTER DATABASE RENAME
[11:15:51.655](0.216s) ok 8 - dynamic bgworker 3 launched
error running SQL: 'psql:<stdin>:1: ERROR: database "renameddb" is being accessed by other users
DETAIL: There is 1 other session using the database.'
while running 'psql --no-psqlrc --no-align --tuples-only --quiet --dbname port=17048 host=/mnt/data/buildfarm/buildroot/tmp/Li3aVcqeUa dbname='postgres' --file - --variable ON_ERROR_STOP=1' with sql 'ALTER DATABASE renameddb SET TABLESPACE test_tablespace' at /mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/../../../../src/test/perl/PostgreSQL/Test/Cluster.pm line 2335.
# Postmaster PID for node "mynode" is 792281
### Stopping node "mynode" using mode immediate
# Running: pg_ctl --pgdata /mnt/data/buildfarm/buildroot/HEAD/pgsql.build/src/test/modules/worker_spi/tmp_check/t_002_worker_terminate_mynode_data/pgdata --mode immediate stop
waiting for server to shut down...... done
server stopped
# No postmaster PID for node "mynode"
[11:16:02.658](11.003s) # Tests were run but no plan was declared and done_testing() was not seen.
[11:16:02.659](0.001s) # Looks like your test exited with 29 just after 8.

2026-03-31 11:15:51.686 UTC [792340:4] 002_worker_terminate.pl LOG: statement: ALTER DATABASE renameddb SET TABLESPACE test_tablespace
2026-03-31 11:15:51.687 UTC [792340:5] 002_worker_terminate.pl DEBUG: attempting worker termination for database 16413
2026-03-31 11:15:51.687 UTC [792340:6] 002_worker_terminate.pl DEBUG: termination requested for worker (PID 792336) on database 16413
2026-03-31 11:15:51.787 UTC [792340:7] 002_worker_terminate.pl DEBUG: attempting worker termination for database 16413
2026-03-31 11:15:51.787 UTC [792340:8] 002_worker_terminate.pl DEBUG: termination requested for worker (PID 792336) on database 16413
2026-03-31 11:15:51.887 UTC [792340:9] 002_worker_terminate.pl DEBUG: attempting worker termination for database 16413
2026-03-31 11:15:51.887 UTC [792340:10] 002_worker_terminate.pl DEBUG: termination requested for worker (PID 792336) on database 16413
<-- snip -->
2026-03-31 11:15:57.088 UTC [792340:102] 002_worker_terminate.pl DEBUG: termination requested for worker (PID 792336) on database 16413
2026-03-31 11:15:57.188 UTC [792340:103] 002_worker_terminate.pl DEBUG: attempting worker termination for database 16413
2026-03-31 11:15:57.188 UTC [792340:104] 002_worker_terminate.pl DEBUG: termination requested for worker (PID 792336) on database 16413
2026-03-31 11:15:57.288 UTC [792340:105] 002_worker_terminate.pl ERROR: database "renameddb" is being accessed by other users
2026-03-31 11:15:57.288 UTC [792340:106] 002_worker_terminate.pl DETAIL: There is 1 other session using the database.
2026-03-31 11:15:57.288 UTC [792340:107] 002_worker_terminate.pl STATEMENT: ALTER DATABASE renameddb SET TABLESPACE test_tablespace
2026-03-31 11:15:57.289 UTC [792340:108] 002_worker_terminate.pl LOG: disconnection: session time: 0:00:05.611 user=buildfarm database=postgres host=[local]
2026-03-31 11:15:57.300 UTC [792281:23] LOG: received immediate shutdown request
2026-03-31 11:15:57.300 UTC [792281:24] DEBUG: updating PMState from PM_RUN to PM_WAIT_BACKENDS
2026-03-31 11:16:02.309 UTC [792281:25] LOG: issuing SIGKILL to recalcitrant children
2026-03-31 11:16:02.547 UTC [792281:26] DEBUG: updating PMState from PM_WAIT_BACKENDS to PM_WAIT_DEAD_END
2026-03-31 11:16:02.547 UTC [792281:27] DEBUG: updating PMState from PM_WAIT_DEAD_END to PM_NO_CHILDREN
2026-03-31 11:16:02.574 UTC [792281:28] LOG: database system is shut down

--
Daniel Gustafsson

---

From: Amit Langote <[email-masked]>
Date: Tue, 31 Mar 2026 21:15:22 +0900

> prion is happy now, the fix works, thanks.

Yep, good.

Because I noticed a use-after-free with prion, I thought to check our
preparedness for CLOBBER_CACHE_ALWAYS and found issues in both the
committed patch (and similar code in 0002): riinfo going stale inside
ri_FastPathCheck() after relation opens and dangling fpmeta pointer
after riinfo invalidation.  0001 fixes those; I'll apply it tomorrow
morning.

0002 is the rebased batching patch.

--
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 31 Mar 2026 15:33:53 +0200

>> Though, I'm not sure if or why the fix is now the reason for

Right, that failure doesn't seem related. It first appeared ~2 weeks
ago, i.e. before this got committed.

I don't know what triggered it. It might even be a simple timing issue.
This is a rpi4 machine, running from a USB flashdrive, so it's pretty
slow, and a processes can occasionally "hang" for a little bit and not
disconnect quick enough.

Not sure if something changed ~2 weeks ago. It might also be the flash
drive getting flaky (even though I don't see anything in dmesg).

Anyway, this is likely unrelated to the commit.

regards

-- 
Tomas Vondra

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 31 Mar 2026 23:54:29 +0800

Hi Amit,

> after riinfo invalidation.  0001 fixes those; I'll apply it tomorrow

+       if (riinfo->fpmeta == NULL)
+       {
+               /* Reload to ensure it's valid. */
+               riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);

I was thinking of wrapping the reload in a conditional check like
`!riinfo->valid`, since `riinfo` can be valid even when `fpmeta == NULL`.
However,  `if (riinfo->fpmeta == NULL)` should rarely be true, so the
unconditional reload is harmless, and the code is cleaner.

+1 to the fix.

> 0002 is the rebased batching patch.

The change of RI_FastPathEntry from storing riinfo to fk_relid
makes sense to me. I'll do another review on 0002 tomorrow.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Wed, 1 Apr 2026 09:06:49 +0900

> drive getting flaky (even though I don't see anything in dmesg).

I see, thanks for checking that.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 1 Apr 2026 17:51:21 +0900

> unconditional reload is harmless, and the code is cleaner.

Thanks for checking.

I have just pushed a slightly modified version of that.

Here's another version.

This time, I have another fixup patch (0001) to make FastPathMeta
self-contained by copying the FmgrInfo structs it needs out of
RI_CompareHashEntry rather than storing pointers into it.  This avoids
any dependency on those cache entries remaining stable.  I'll push
that once the just committed patch has seen enough BF animals.

0002 is rebased over that.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 1 Apr 2026 18:51:10 +0900

> that once the just committed patch has seen enough BF animals.

Pushed.

> 0002 is rebased over that.

Rebased again.

-- 
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 1 Apr 2026 19:56:10 +0800

> > 0002 is rebased over that.

+static void
+ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel)
+{
+ /* Reload; may have been invalidated since last batch accumulation. */
+ const RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(fpentry->conoid);

...
+ if (riinfo->fpmeta == NULL)
+ {
+ /* Reload to ensure it's valid. */
+ riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);
+ ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
+   fk_rel, idx_rel);
+ }

ri_LoadConstraintInfo is currently invoked twice within
ri_FastPathBatchFlush. Should we eliminate the second call?

Alternatively, we could refactor ri_FastPathBatchFlush to accept
an additional parameter, `const RI_ConstraintInfo *riinfo`, so we
can remove the need for the first call. In that case, we need to call
ri_LoadConstraintInfo in ri_FastPathEndBatch.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Wed, 1 Apr 2026 21:18:14 +0900

> ri_FastPathBatchFlush. Should we eliminate the second call?

I think we can't because the entry may be stale by the time we get to
the ri_populate_fastpath_metadata() call due to intervening steps;
even something as benign-looking index_beginscan() may call code paths
that can trigger invalidation in rare cases. Maybe predictably in
CLOBBER_CACHE_ALWAYS builds.

> ri_LoadConstraintInfo in ri_FastPathEndBatch.

Yeah, I think that's fine.  Done that way in the attached.

Also, I realized that we could do:

@@ -2937,7 +2937,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry,
Relation fk_rel)
                                       fk_rel, idx_rel);
     }
     Assert(riinfo->fpmeta);
-    if (riinfo->nkeys == 1)
+    if (riinfo->nkeys == 1 && fpentry->batch_count > 1)
         violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo,
                                                 fk_rel, snapshot, scandesc);

so that the fixed overhead of ri_FastPathFlushArray (allocating
matched[] array on stack and constructing ArrayType, etc.) is not paid
unnecessarily for single-row batches.

Attached patch is updated that way.

I will continue looking at this tomorrow morning with the aim of
committing it by EOD or Friday.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 2 Apr 2026 16:41:30 +0900

> unnecessarily for single-row batches.

There's another case in which it is not ok to use FlushArray and that
is if the index AM's amsearcharray is false (should be true in all
cases because the unique index used for PK is always btree).  Added an
Assert to that effect next to where SK_SEARCHARRAY is set in
ri_FastPathFlushArray rather than a runtime check in the dispatch
condition.

Patch updated.  Also added a comment about invalidation requirement or
lack thereof for RI_FastPathEntry, rename AfterTriggerBatchIsActive()
to simply AfterTriggerIsActive(), fixed the comments in trigger.h
describing the callback mechanism.

Will push tomorrow morning (Friday) barring objections.

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Thu, 2 Apr 2026 15:59:37 +0800

> On Apr 2, 2026, at 15:41, Amit Langote <[email-masked]> wrote:
> 
> <v17-0001-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch>

With a quick eyeball review, I found a typo:
```
+ * relcache invalidation.  The entry itself is torn down at batch at batch end
```

There are two “at batch”.

I plan to spend time testing and tracing this patch tomorrow. But I don’t want to block your progress, if I find anything, I will report to you.

Best regards,

---

From: Amit Langote <[email-masked]>
Date: Fri, 3 Apr 2026 14:52:45 +0900

Hi,

> There are two “at batch”.

Thanks for spotting that.  Fixed and pushed.

> I plan to spend time testing and tracing this patch tomorrow. But I don’t want to block your progress, if I find anything, I will report to you.

Sure, I didn't want to leave committing this to the weekend or the next week.

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Fri, 3 Apr 2026 16:57:36 +0800

> On Apr 3, 2026, at 13:52, Amit Langote <[email-masked]> wrote:
> 
> Hi,
> 

Hi Amit,

I spent several hours debugging this patch today, and I found a problem where the batch mode doesn't seem to handle deferred RI triggers, although the commit message suggests that it should.

I traced this scenario:
```
CREATE TABLE pk (a int primary key);
CREATE TABLE fk (a int references pk(a) DEFERRABLE INITIALLY DEFERRED);
BEGIN;
INSERT INTO fk VALUES (1);
INSERT INTO pk VALUES (1);
COMMIT;
```

When COMMIT is executed, it reaches RI_FKey_check(), where AfterTriggerIsActive() checks whether afterTriggers.query_depth >= 0. But in the deferred case, afterTriggers.query_depth is -1.

From the code:
```
	if (ri_fastpath_is_applicable(riinfo))
	{
		if (AfterTriggerIsActive())
		{
			/* Batched path: buffer and probe in groups */
			ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
		}
		else
		{
			/* ALTER TABLE validation: per-row, no cache */
			ri_FastPathCheck(riinfo, fk_rel, newslot);
		}
		return PointerGetDatum(NULL);
	}
```

So this ends up falling back to the per-row path for deferred RI checks at COMMIT, even though the intent here seems to be only to bypass the ALTER TABLE validation case, where batch callbacks would never fire, and MyTriggerDepth is 0. So, maybe we can just check MyTriggerDepth>0 in AfterTriggerIsActive().

I tried the attached fix. With it, deferred triggers go through the batch mode, and all existing tests still pass. But I am still new to PG development, so I’m not sure whether I may have missed something.

Best regards,

---

From: Amit Langote <[email-masked]>
Date: Fri, 3 Apr 2026 18:39:48 +0900

On Fri, Apr 3, 2026 at 5:58 PM Chao Li <[email-masked]> wrote:
> I tried the attached fix. With it, deferred triggers go through the batch mode, and all existing tests still pass.

I think you might be right.  Thanks for the patch.  It looks correct
to me at a glance, but I will need to check it a bit more closely
before committing.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 6 Apr 2026 18:45:05 +0900

> to me at a glance, but I will need to check it a bit more closely

Thinking about this some more, your fix is on the right track but
needs a bit more work -- MyTriggerDepth > 0 is too broad since it
fires for BEFORE triggers too. I have a revised version using a new
afterTriggerFiringDepth counter that I'll push shortly.

Added an open item for tracking in the meantime:
https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Tue, 7 Apr 2026 09:45:42 +0800

> On Apr 6, 2026, at 17:45, Amit Langote <[email-masked]> wrote:
> 
> <v2-0001-Fix-deferred-FK-check-batching-introduced-by-comm.patch>

V2 looks good to me. Besides the normal cases, I also traced an abnormal case to verify that afterTriggerFiringDepth is always reset to 0:
```
evantest=# begin;
BEGIN
evantest=*# INSERT INTO fk VALUES (2);
INSERT 0 1
evantest=*# commit;
ERROR:  insert or update on table "fk" violates foreign key constraint "fk_a_fkey"
DETAIL:  Key (a)=(2) is not present in table "pk".
```

Best regards,

---

From: Amit Langote <[email-masked]>
Date: Tue, 7 Apr 2026 11:12:15 +0900

> DETAIL:  Key (a)=(2) is not present in table "pk".

Thanks for checking.  Pushed.

-- 
Thanks, Amit Langote

---

From: Evan Montgomery-Recht <[email-masked]>
Date: Tue, 7 Apr 2026 08:59:59 -0400

Hi Amit,

First time contributing to this project, let me know if I missed
something or need to adjust what I put together.

I found a crash in the RI fast-path FK check code introduced by
2da86c1ef9b and extended by b7b27eb41a5. C-language extensions that
use SPI to INSERT into tables with multiple FK constraints hit an
assertion failure (or, without assertions, a server crash) when the
batch callback fires. I discovered this via PostGIS's topology CI --
toTopoGeom() uses SPI to insert into edge_data, which has 4 immediate
FK constraints referencing node and face. PG 18 passes the same test;
the master crashes.

This appears to be a separate issue from Chao Li's deferred-trigger
batching bug; the patches touch different files and don't conflict.  I
did do a regression test on the merge referenced both PostgreSQL and
PostGIS (to validate that this works.)

The problem: ri_FastPathGetEntry() opens pk_rel/idx_rel and creates
TupleTableSlots, registering them with the current resource owner --
the SPI portal's. The batch callback ri_FastPathTeardown() only fires
when query_depth == 0 (via FireAfterTriggerBatchCallbacks), but by
that time the inner portal has finished and its resource owner has
released the relation references and TupleDesc pins. The teardown then
tries to close relations whose refcounts are already zero:

TRAP: failed Assert("rel->rd_refcnt > 0"), File: "relcache.c"

RelationDecrementReferenceCount
-> RelationClose -> index_close -> ri_FastPathTeardown
-> ri_FastPathEndBatch -> FireAfterTriggerBatchCallbacks
-> AfterTriggerEndQuery -> standard_ExecutorFinish

and TupleDesc pins that are no longer tracked by any resource owner
cause "tupdesc reference not owned by resource owner" errors.

Note that simple PL/pgSQL functions don't trigger this because
PL/pgSQL's SPI connection spans the entire function call, so the
portal's resource owner outlives the batch callback. The crash
requires nested SPI from a C extension, which creates a shorter-lived
portal.

The attached patch (against master, for application) fixes this by
transferring both relation references and TupleDesc pins from the
current resource owner to CurTransactionResourceOwner immediately
after creating them in ri_FastPathGetEntry(). The transfer uses
RelationIncrementReferenceCount / PinTupleDesc under the target owner
followed by RelationDecrementReferenceCount / ReleaseTupleDesc under
the original. I chose this over switching CurrentResourceOwner around
the table_open/index_open calls because the latter also affects
transient buffer pins acquired during catalog lookups inside those
functions.

ri_FastPathTeardown is updated to clear any buffered tuples (whose
buffer pins belong to the current resource owner) before switching to
CurTransactionResourceOwner for the close/drop operations.

The patch also adds a test module (test_spi_func) with a C function
that executes SQL via SPI_connect/SPI_execute/SPI_finish, since this
crash cannot be triggered from PL/pgSQL. The test exercises the
C-level SPI INSERT with multiple FK constraints, FK violations, and
nested PL/pgSQL-calls-C-SPI (matching the PostGIS call pattern).

This is purely a correctness fix with no performance or backward
compatibility impact. No documentation changes are needed since this
is an internal bug fix.

The patch compiles cleanly and passes pgindent, clang-tidy, and
cppcheck. All 247 regression subtests pass, along with the full meson
test suite (370 ok, 0 fail, 21 skipped) (skipped due to hardware
availability on my side this week). I also verified the PostGIS
topology test (toTopoGeom) passes clean with no warnings, and tested
abort paths (FK violation, transaction rollback, subtransaction abort
via EXCEPTION blocks) (not in scope of PostgreSQL but more for my own
verification that things work). Code coverage on the new lines is
100%. Tested on macOS (aarch64) and Linux (aarch64, via Docker).

Unrelated to my patch, SonarCloud flagged a potential issue in
recheck_matched_pk_tuple() (line 3370): the function loops over
ii_NumIndexKeyAttrs elements of the skeys array, but the caller in
ri_FastPathFlushArray passes recheck_skey[1] -- an array of exactly
one element. This is safe because ri_FastPathFlushArray is the

single-column FK path, so ii_NumIndexKeyAttrs is always 1 there.
However, the function signature doesn't communicate this constraint,
which flags as CWE-125 (out-of-bounds read) / CERT C ARR30-C. Adding
an nkeys parameter (like ri_FastPathProbeOne already has) would make
the contract explicit.

Unrelated to PostgreSQL directly, I currently have a workaround for
the changes I'm making to PostGIS to test out some performance
enhancements. I left comments in the code so that if this gets
accepted, I can revert to a cleaner approach, as this appears to only
affect pg19 based on the testing I've done so far.

If there's a cleaner approach or a larger underlying issue, I'm
definitely willing to keep testing to find a better solution.

--
thanks, Evan Montgomery-Recht

---

From: Amit Langote <[email-masked]>
Date: Wed, 8 Apr 2026 10:23:59 +0900

Hi Evan,

> 100%. Tested on macOS (aarch64) and Linux (aarch64, via Docker).

Thanks for the report and the patch.

I'll need to study this one a bit more closely.  Added an open item
for the time being:
https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

> an nkeys parameter (like ri_FastPathProbeOne already has) would make

Makes sense.  Will push the attached patch for this.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 8 Apr 2026 18:58:23 +0900

> > nested PL/pgSQL-calls-C-SPI (matching the PostGIS call pattern).

I applied only the test module changes and it passes (without
crashing) even without your proposed fix. It seems that's because the
C function in test_spi_func calling SPI is using the same resource
owner as the parent SELECT. I think you'd need to create a resource
owner manually in the spi_exec() C function to reproduce the crash, as
done in the attached 0001, which contains the src/test changes
extracted from your patch modified as described, including renaming
the C function to spi_exec_sql().

Also, the test cases that call spi_exec() (_sql()) directly from a
SELECT don't actually exercise the crash path because there is no
outer trigger-firing loop active. query_depth is 0 inside the inner
SPI's AfterTriggerEndQuery, so the old guard wouldn't suppress the
callback there anyway. The critical case requires spi_exec_sql() to be
called from inside an AFTER trigger, where query_depth > 0 causes the
guard to defer the callback past the inner resource owner's lifetime.
I've added that test case. I kept your original test cases as they
still provide useful coverage of C-level SPI FK behavior even if they
don't exercise the crash path specifically.  Maybe your original
PostGIS test suite that hit the crash did have the right structure,
but that's not reflected in the patch as far as I can tell.

I've also renamed the module to test_spi_resowner to better reflect
what it's about.

For the fix, I have a different proposal. As you observed, the
query_depth > 0 early return in FireAfterTriggerBatchCallbacks() means
that the nested SPI's callbacks get called under the outer resource
owner, which may not be the same as the one that SPI used. I think it
was a mistake to have that early return in the first place. Instead we
could remember for each callback what firing level it should be called
at, so the nested SPI's callbacks fire before returning to the parent
level and parent-level callbacks fire when the parent level completes.
I have implemented that in the attached 0002 along with transaction
boundary cleanup of callbacks, which passes the check-world for me,
but I'll need to stare some more at it before committing.

Let me know if this also fixes your own in-house test suite or if you
have any other suggestions or if you think I am missing something.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 8 Apr 2026 23:26:21 +0900

> have any other suggestions or if you think I am missing something.

One more cleanup patch attached as 0003: afterTriggerFiringDepth was
added by commit 5c54c3ed1 as a file-static variable, which in
hindsight should have been a field in AfterTriggersData alongside the
other per-transaction after-trigger state. This patch makes that
correction.

One alternative design worth considering for 0002: storing
batch_callbacks per query level in AfterTriggersQueryData rather than
as a single list in AfterTriggersData, so callbacks naturally live at
the query level where they were registered and get cleaned up with
AfterTriggerFreeQuery on abort. Deferred constraints still need a
top-level list in AfterTriggersData since they fire outside any query
level. FireAfterTriggerBatchCallbacks() takes a list parameter and the
caller passes either the query-level or top-level list as appropriate.
This eliminates the need for firing_depth-matched firing entirely. I
did that in 0004.  I think I like it over 0002.  Will look more
closely tomorrow morning.

--
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Thu, 9 Apr 2026 15:39:44 +0800

> On Apr 8, 2026, at 22:26, Amit Langote <[email-masked]> wrote:
> 
> <v3-0004-Store-batch-callbacks-at-the-appropriate-level-ra.patch><v3-0002-Fix-RI-fast-path-crash-under-nested-C-level-SPI.patch><v3-0003-Move-afterTriggerFiringDepth-into-AfterTriggersDa.patch><v3-0001-Modified-test-suite-from-Evan-s-patch.patch>

A few comments on v3:

1 - 0002
```
 static void
 FireAfterTriggerBatchCallbacks(void)
 {
+	List	   *remaining = NIL;
+	List	   *to_fire = NIL;
 	ListCell   *lc;
 
-	if (afterTriggers.query_depth > 0)
-		return;
+	/* remaining and to_fire lists must survive until callbacks complete */
+	MemoryContext oldcxt = MemoryContextSwitchTo(TopTransactionContext);
```

I think remaining and to_fire should stay in the same context of afterTriggers.batch_callbacks, so instead of hard coding TopTransactionContext, we can use GetMemoryChunkContext(afterTriggers.batch_callbacks), which makes the intention explicit.

2 - 0004, I noticed one potential problem, although I am not sure whether it can really happen in practice. This version stores callback items at the individual query depth, and FireAfterTriggerBatchCallbacks() now iterates the callback list for that depth and invokes each callback directly. My concern is that if one of those callbacks needs to register a new callback, that would append a new item to the same list while it is being iterated. That seems unsafe to me, because list append may create a new list structure underneath. If that happens, we may end up modifying the list being traversed, which does not look safe.

This problem doesn’t exist in 0002, because 0002 splits afterTriggers.batch_callbacks into remaining and to_fire, and reset afterTriggers.batch_callbacks = remaining before running callbacks. But the problem is, if a callback registers a new callback, the new callback goes to afterTriggers.batch_callbacks, so it won’t get executed.

From this perspective, I would assume a callback should not be allowed to register a new callback. Can you please help confirm?

Best regards,

---

From: Amit Langote <[email-masked]>
Date: Thu, 9 Apr 2026 17:40:49 +0900

Hi,

> > closely tomorrow morning.

Thanks for the review.

> I think remaining and to_fire should stay in the same context of afterTriggers.batch_callbacks, so instead of hard coding TopTransactionContext, we can use GetMemoryChunkContext(afterTriggers.batch…

I'm dropping 0002 or have merged 0004 into it so this memory context
switch is no longer present.

> From this perspective, I would assume a callback should not be allowed to register a new callback. Can you please help confirm?

Good point on the re-entrant registration concern. I've added a
firing_batch_callbacks flag to AfterTriggersData that prevents
callbacks from registering new callbacks during
FireAfterTriggerBatchCallbacks(), with an Assert in
RegisterAfterTriggerBatchCallback() to enforce it. That should keep
the list being iterated from being modified.

The attached patches are updated accordingly. 0001 is the main fix
incorporating the per-query-level storage design, the transaction
boundary cleanup, and the firing_batch_callbacks guard. 0002 is a
followup that moves afterTriggerFiringDepth into AfterTriggersData as
a minor cleanup of 5c54c3ed1b9. Barring further feedback I plan to
commit 0001 and 0002 shortly. For 0003, I need to check on the policy
around adding new test modules during feature freeze before committing
it.

-- 
Thanks, Amit Langote

---

From: jie wang <[email-masked]>
Date: Thu, 9 Apr 2026 17:21:51 +0800

Amit Langote <[email-masked]> 于2026年4月9日周四 16:41写道：

> > +       MemoryContext oldcxt =
> MemoryContextSwitchTo(TopTransactionContext);
> > ```
> >
> > I think remaining and to_fire should stay in the same context of
> afterTriggers.batch_callbacks, so instead of hard coding
> TopTransactionContext, we can use
> GetMemoryChunkContext(afterTriggers.batch_callbacks), which makes the
> intention explicit.
>
> > 2 - 0004, I noticed one potential problem, although I am not sure
> whether it can really happen in practice. This version stores callback
> items at the individual query depth, and FireAfterTriggerBatchCallbacks()
> now iterates the callback list for that depth and invokes each callback
> directly. My concern is that if one of those callbacks needs to register a
> new callback, that would append a new item to the same list while it is
> being iterated. That seems unsafe to me, because list append may create a
> new list structure underneath. If that happens, we may end up modifying the
> list being traversed, which does not look safe.
> >
> > This problem doesn’t exist in 0002, because 0002 splits
> afterTriggers.batch_callbacks into remaining and to_fire, and reset
> afterTriggers.batch_callbacks = remaining before running callbacks. But the
> problem is, if a callback registers a new callback, the new callback goes
> to afterTriggers.batch_callbacks, so it won’t get executed.
> >
> > From this perspective, I would assume a callback should not be allowed
> to register a new callback. Can you please help confirm?
>

 Hi,

I took a glance at the patch, overall looks good to me. A nitpick on 0001:

+       bool            firing_batch_callbacks; /* true when in
+
     * FireAfterTriggersBatchCallbacks() */

Looks like a typo in the comment. The function name is
FireAfterTriggerBatchCallbacks, no “s” after Trigger.

Best regards,
--
wang jie

---

From: Amit Langote <[email-masked]>
Date: Thu, 9 Apr 2026 18:24:47 +0900

On Thu, Apr 9, 2026 at 6:22 PM jie wang <[email-masked]> wrote:
> +                                                                               * FireAfterTriggersBatchCallbacks() */
>
> Looks like a typo in the comment. The function name is FireAfterTriggerBatchCallbacks, no “s” after Trigger.

Thanks, I've fixed the typo in my local tree.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 9 Apr 2026 18:29:04 +0900

> Makes sense.  Will push the attached patch for this.

Pushed this fix.

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Thu, 9 Apr 2026 18:25:21 +0800

> On Apr 9, 2026, at 16:40, Amit Langote <[email-masked]> wrote:
> 
> Hi,
> 
> <v4-0002-Move-afterTriggerFiringDepth-into-AfterTriggersDa.patch><v4-0001-Fix-RI-fast-path-crash-under-nested-C-level-SPI.patch><v4-0003-Add-test-module-for-RI-fast-path-FK-checks-under-.patch>

0001 and 0002 look good to me. I didn’t review 0003 and don’t intend to review it.

Best regards,

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 9 Apr 2026 13:07:50 +0200

On Mon, Mar 02, 2026 at 01:34:41PM +0100, Tomas Vondra wrote:
> 
> TBH I haven't noticed the memory context issue myself, I only noticed
> because the builds with index prefetch started crashing.

We're getting a crash in PostGIS too, since that commit was merged into
the master branch, see https://trac.osgeo.org/postgis/ticket/6066

The crash is triggered a C function using SPI.

--strk;

---

From: Amit Langote <[email-masked]>
Date: Thu, 9 Apr 2026 20:55:01 +0900

Hi Sandro,

> The crash is triggered a C function using SPI.

Evan Montgomery-Recht posted a report of the same issue on this thread
a couple of days ago.

I have posted a patch to fix the issue, which I will commit tomorrow
after a bit more testing.

-- 
Thanks, Amit Langote

---

From: Sandro Santilli <[email-masked]>
Date: Thu, 9 Apr 2026 18:01:58 +0200

> Evan Montgomery-Recht posted a report of the same issue on this thread

I confirm the patch attached in Evan's email [1] fixes the crash for us.

[1] https://www.postgresql.org/message-id/CAEg7pwcKf01FmDqFAf-Hzu_pYnMYScY_Otid-pe9uw3BJ6gq9g%40mail.gmail.com

> after a bit more testing.

I also confirm your patch v4-0001-Fix-RI-fast-path-crash-under-nested-C-level-SPI.patch
fixes the crash for us. Thank you !

Let me know when it is time to test again against master.

--strk;

---

From: Amit Langote <[email-masked]>
Date: Fri, 10 Apr 2026 13:14:11 +0900

Hi Sandro,

> fixes the crash for us. Thank you !

Thanks for confirming that.

> Let me know when it is time to test again against master.

I have just pushed 0001 which you'll find in master as 34a3078629.

-- 
Thanks, Amit Langote

---

From: Chao Li <[email-masked]>
Date: Fri, 10 Apr 2026 12:20:24 +0800

> On Apr 10, 2026, at 12:14, Amit Langote <[email-masked]> wrote:
> 
> Hi Sandro,
> 

Hi Amit, looks like you missed to fix the typo that Jie pointed out. In 34a307862930056e1976471d6d81a5e2efc148df,
```
+ bool firing_batch_callbacks; /* true when in
+ * FireAfterTriggersBatchCallbacks() */
```
The typo is still there.

Best regards,

---

From: Amit Langote <[email-masked]>
Date: Fri, 10 Apr 2026 13:21:37 +0900

> + * FireAfterTriggersBatchCallbacks() */

Yep, my bad.  Will push a fix shortly.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 10 Apr 2026 16:39:16 +0900

> 0001 and 0002 look good to me. I didn’t review 0003 and don’t intend to review it.

I've now pushed 0001 (34a3078629) and 0002 (d6e96bacd3c).

Here's the remaning patch to add src/test/modules/test_spi_resowner
rebased against master. I'm holding off on committing the test module
until I confirm the policy on new test modules during feature freeze.
It's also worth discussing whether this is the right place for testing
C extensions that use SPI with a dedicated resource owner, or whether
that coverage belongs elsewhere.

-- 
Thanks, Amit Langote

---

From: Sandro Santilli <[email-masked]>
Date: Fri, 10 Apr 2026 20:35:36 +0200

> I have just pushed 0001 which you'll find in master as 34a3078629.

No crash with commit 2a3d2f9f68da0c430c497bf29f60373f5214307d
(which includes 34a3078629).

Thank you !

--strk;

  Libre GIS consultant/developer 🎺
  https://strk.kbt.io/services.html

---

From: Haibo Yan <[email-masked]>
Date: Fri, 10 Apr 2026 16:34:21 -0700

> that coverage belongs elsewhere.

I reviewed the patch, and overall it looks close. I have a few comments:

   1.

   Should spi_exec_sql() be made exception-safe?

   The current implementation does not restore CurrentResourceOwner or
   release/delete childowner on all error paths, and it also does not check
   for SPI_connect() failure. Since this module is specifically meant to
   exercise ResourceOwner lifetime interactions, I think the helper itself
   should be robust in both success and error paths.
   2.

   Consider adding a follow-up test that does failure first, then success.

   That would help show that the helper does not leave any lingering state
   behind after an error.
   3.

   Consider trimming the long explanatory comments in the regression test a
   bit.

   The rationale is useful, but some of it is repeated across the commit
   message, the SQL file header, and the expected output.

Regards
Haibo

---

From: Amit Langote <[email-masked]>
Date: Wed, 15 Apr 2026 15:03:50 +0900

On Sat, Apr 11, 2026 at 8:34 AM Haibo Yan <[email-masked]> wrote:
> On Fri, Apr 10, 2026 at 12:39 AM Amit Langote <[email-masked]> wrote:
> The current implementation does not restore CurrentResourceOwner or release/delete childowner on all error paths, and it also does not check for SPI_connect() failure. Since this module is specifically meant to exercise ResourceOwner lifetime interactions, I think the helper itself should be robust in both success and error paths.
>
> That would help show that the helper does not leave any lingering state behind after an error.
>
> Consider trimming the long explanatory comments in the regression test a bit.
>
> The rationale is useful, but some of it is repeated across the commit message, the SQL file header, and the expected output.

Thanks Haibo for the review. Your points are well taken and would need
to be addressed if this module were to be committed, but I've been
reconsidering whether to commit it at all. It was written to reproduce
a specific crash caused by an extension's idiosyncratic use of SPI
with a dedicated resource owner, a pattern that's specific to PostGIS
and similar extensions rather than something core PostgreSQL
exercises. Now that the crash is fixed, the module's main value is as
a regression test for that one scenario. I'm not convinced it pulls
its weight as a permanent addition to the test suite, especially given
the maintenance burden and the time it adds to test runs.

I'll hold off on committing it unless someone feels strongly that it
should be included.

-- 
Thanks, Amit Langote

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 20 Apr 2026 22:50:29 +0200

> Will push tomorrow morning (Friday) barring objections.

This commit contains a couple of calls

ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
                               fk_rel, idx_rel);

where the cast casts away the const-ness of riinfo.

But this is kind of a lie, since the purpose of 
ri_populate_fastpath_metadata() is to modify riinfo.

I think the right thing to do here is to unwind the const qualifiers up 
the stack.  See attached patch.

From 35f273c812aa4f1345a3c1a9eb1443e3c7439254 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Mon, 20 Apr 2026 22:40:05 +0200
Subject: [PATCH] Fix some const qualifier use in ri_triggers.c

---
 src/backend/utils/adt/ri_triggers.c | 34 ++++++++++++++---------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index e060280fcd4..f63a7f0b580 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -297,9 +297,9 @@ static RI_CompareHashEntry *ri_HashCompareOp(Oid eq_opr, Oid typeid);
 
 static void ri_CheckTrigger(FunctionCallInfo fcinfo, const char *funcname,
 							int tgkind);
-static const RI_ConstraintInfo *ri_FetchConstraintInfo(Trigger *trigger,
-													   Relation trig_rel, bool rel_is_pk);
-static const RI_ConstraintInfo *ri_LoadConstraintInfo(Oid constraintOid);
+static RI_ConstraintInfo *ri_FetchConstraintInfo(Trigger *trigger,
+												 Relation trig_rel, bool rel_is_pk);
+static RI_ConstraintInfo *ri_LoadConstraintInfo(Oid constraintOid);
 static Oid	get_ri_constraint_root(Oid constrOid);
 static SPIPlanPtr ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes,
 							   RI_QueryKey *qkey, Relation fk_rel, Relation pk_rel);
@@ -309,12 +309,12 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
 							TupleTableSlot *oldslot, TupleTableSlot *newslot,
 							bool is_restrict,
 							bool detectNewRows, int expect_OK);
-static void ri_FastPathCheck(const RI_ConstraintInfo *riinfo,
+static void ri_FastPathCheck(RI_ConstraintInfo *riinfo,
 							 Relation fk_rel, TupleTableSlot *newslot);
-static void ri_FastPathBatchAdd(const RI_ConstraintInfo *riinfo,
+static void ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
 								Relation fk_rel, TupleTableSlot *newslot);
 static void ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
-								  const RI_ConstraintInfo *riinfo);
+								  RI_ConstraintInfo *riinfo);
 static int	ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot,
 								  const RI_ConstraintInfo *riinfo, Relation fk_rel,
 								  Snapshot snapshot, IndexScanDesc scandesc);
@@ -357,7 +357,7 @@ static void ri_FastPathTeardown(void);
 static Datum
 RI_FKey_check(TriggerData *trigdata)
 {
-	const RI_ConstraintInfo *riinfo;
+	RI_ConstraintInfo *riinfo;
 	Relation	fk_rel;
 	Relation	pk_rel;
 	TupleTableSlot *newslot;
@@ -2341,11 +2341,11 @@ ri_CheckTrigger(FunctionCallInfo fcinfo, const char *funcname, int tgkind)
 /*
  * Fetch the RI_ConstraintInfo struct for the trigger's FK constraint.
  */
-static const RI_ConstraintInfo *
+static RI_ConstraintInfo *
 ri_FetchConstraintInfo(Trigger *trigger, Relation trig_rel, bool rel_is_pk)
 {
 	Oid			constraintOid = trigger->tgconstraint;
-	const RI_ConstraintInfo *riinfo;
+	RI_ConstraintInfo *riinfo;
 
 	/*
 	 * Check that the FK constraint's OID is available; it might not be if
@@ -2395,7 +2395,7 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation trig_rel, bool rel_is_pk)
 /*
  * Fetch or create the RI_ConstraintInfo struct for an FK constraint.
  */
-static const RI_ConstraintInfo *
+static RI_ConstraintInfo *
 ri_LoadConstraintInfo(Oid constraintOid)
 {
 	RI_ConstraintInfo *riinfo;
@@ -2777,7 +2777,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
  * ri_FastPathBatchAdd().
  */
 static void
-ri_FastPathCheck(const RI_ConstraintInfo *riinfo,
+ri_FastPathCheck(RI_ConstraintInfo *riinfo,
 				 Relation fk_rel, TupleTableSlot *newslot)
 {
 	Relation	pk_rel;
@@ -2820,8 +2820,7 @@ ri_FastPathCheck(const RI_ConstraintInfo *riinfo,
 	{
 		/* Reload to ensure it's valid. */
 		riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);
-		ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
-									  fk_rel, idx_rel);
+		ri_populate_fastpath_metadata(riinfo, fk_rel, idx_rel);
 	}
 	Assert(riinfo->fpmeta);
 	ri_ExtractValues(fk_rel, newslot, riinfo, false, pk_vals, pk_nulls);
@@ -2857,7 +2856,7 @@ ri_FastPathCheck(const RI_ConstraintInfo *riinfo,
  * ri_FastPathEndBatch().
  */
 static void
-ri_FastPathBatchAdd(const RI_ConstraintInfo *riinfo,
+ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo,
 					Relation fk_rel, TupleTableSlot *newslot)
 {
 	RI_FastPathEntry *fpentry = ri_FastPathGetEntry(riinfo, fk_rel);
@@ -2884,7 +2883,7 @@ ri_FastPathBatchAdd(const RI_ConstraintInfo *riinfo,
  */
 static void
 ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
-					  const RI_ConstraintInfo *riinfo)
+					  RI_ConstraintInfo *riinfo)
 {
 	Relation	pk_rel = fpentry->pk_rel;
 	Relation	idx_rel = fpentry->idx_rel;
@@ -2941,8 +2940,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel,
 	{
 		/* Reload to ensure it's valid. */
 		riinfo = ri_LoadConstraintInfo(riinfo->constraint_id);
-		ri_populate_fastpath_metadata((RI_ConstraintInfo *) riinfo,
-									  fk_rel, idx_rel);
+		ri_populate_fastpath_metadata(riinfo, fk_rel, idx_rel);
 	}
 	Assert(riinfo->fpmeta);
 
@@ -4147,7 +4145,7 @@ ri_FastPathEndBatch(void *arg)
 		if (entry->batch_count > 0)
 		{
 			Relation	fk_rel = table_open(entry->fk_relid, AccessShareLock);
-			const RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
+			RI_ConstraintInfo *riinfo = ri_LoadConstraintInfo(entry->conoid);
 
 			ri_FastPathBatchFlush(entry, fk_rel, riinfo);
 			table_close(fk_rel, NoLock);
-- 
2.53.0

---

From: Amit Langote <[email-masked]>
Date: Tue, 21 Apr 2026 09:52:55 +0900

> the stack.  See attached patch.

Thanks for the patch.  LGTM.

Are you planning to push it or do you want me to?

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 22 Apr 2026 13:04:17 +0900

Pushed.

-- 
Thanks, Amit Langote