Thread
Commits
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix some const qualifier use in ri_triggers.c
- 1b9dc2cb7574 19 (unreleased) landed
-
doc: Improve consistency of parallel vacuum description.
- 2a3d2f9f68da 19 (unreleased) cited
-
Move afterTriggerFiringDepth into AfterTriggersData
- d6e96bacd3c0 19 (unreleased) landed
-
Fix typo left by 34a30786293
- 03029409b409 19 (unreleased) landed
-
Fix RI fast-path crash under nested C-level SPI
- 34a307862930 19 (unreleased) landed
-
Add nkeys parameter to recheck_matched_pk_tuple()
- e1cc57fabd6c 19 (unreleased) landed
-
Fix deferred FK check batching introduced by commit b7b27eb41a5
- 5c54c3ed1b93 19 (unreleased) landed
-
Optimize fast-path FK checks with batched index probes
- b7b27eb41a5c 19 (unreleased) landed
-
Make FastPathMeta self-contained by copying FmgrInfo structs
- edee5634564d 19 (unreleased) landed
-
Add fast path for foreign key constraint checks
- 2da86c1ef9b5 19 (unreleased) landed
-
Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2024-12-20T04:23:35Z
Hi, 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. In short, performing the RI checks for inserts and updates of a referencing table as direct scans of the PK index results in up to 40% improvement in their performance, especially when they are done in a bulk manner as shown in the following example: create unlogged table pk (a int primary key); insert into pk select generate_series(1, 10000000); insert into fk select generate_series(1, 10000000); On my machine, the last query took 20 seconds with master, whereas 12 seconds with the patches. With master, a significant portion of the time can be seen spent in ExecutorStart() and ExecutorEnd() on the plan for the RI query, which adds up as it's done for each row in a bulk load. Patch avoids that overhead because it calls the index AM directly. The patches haven't changed in the basic design since the last update at [2], though there are few changes: 1. I noticed a few additions to the RI trigger functions the patch touches, such as those to support temporal foreign keys. I decided to leave the SQL for temporal queries in place as the plan for those doesn't look, on a glance, as simple as a simple index scan. 2. As I mentioned in [3], the new way of doing the PK lookup didn't have a way to recheck the PK tuple after detecting concurrent updates of the PK, so would cause an error under READ COMMITTED isolation level. The old way of executing an SQL plan would deal with that using the EvalPlanQual() mechanism in the executor. In the updated patch, I've added an equivalent rechecking function that's called in the same situations as EvalPlanQual() would get called in the old method. 3. I reordered the patches as Robert suggested at [5]. Mainly because the patch set includes changes to address a bug where PK lookups could return incorrect results under the REPEATABLE READ isolation level. This issue arises because RI lookups on partitioned PK tables manipulate ActiveSnapshot to pass the snapshot that's used by find_inheritance_children() to determine the visibility of detach-pending partitions to these RI lookups. To address this, the patch set introduces refactoring of the PartitionDesc interface, included in patch 0001. This refactoring eliminates the need to manipulate ActiveSnapshot by explicitly passing the correct snapshot for detach-pending visibility handling. The main patch (0002+0003), which focuses on improving performance by avoiding SQL queries for RI checks, builds upon these refactoring changes to pass the snapshot directly instead of manipulating the ActiveSnapshot. Reordering the patches this way ensures a logical progression of changes, as Robert suggested, while avoiding any impression that the bug was introduced by the ri_triggers.c changes. However, I need to spend some time addressing Robert's feedback on the basic design, as outlined at [5]. Specifically, the new PK lookup function could benefit significantly from caching information rather than recomputing it for each row. This implies that the PlanCreate function should create a struct to store reusable information across PlanExecute calls for different rows being checked. Beyond implementing these changes, I also need to confirm that the new plan execution preserves all operations performed by the SQL plan for the same checks, particularly those affecting user-visible behavior. I've already verified that permission checks are preserved: revoking access to the PK table during the checks causes them to fail, as expected. This behavior is maintained because permission checks are performed during each execution. The planned changes to separate the "plan" and "execute" steps should continue to uphold this and other behaviors that might need to be preserved. -- Thanks, Amit Langote [1] Simplifying foreign key/RI checks: https://www.postgresql.org/message-id/flat/CA%2BHiwqG5e8pk8s7%2B7zhr1Nc_PGyhEdM5f%3DpHkMOdK1RYWXfJsg%40mail.gmail.com [2] Eliminating SPI from RI triggers - take 2 https://www.postgresql.org/message-id/flat/CA%2BHiwqG5e8pk8s7%2B7zhr1Nc_PGyhEdM5f%3DpHkMOdK1RYWXfJsg%40mail.gmail.com [3] https://www.postgresql.org/message-id/CA+TgmoaiTNj4DgQy42OT9JmTTP1NWcMV+ke0i=+a7=VgnzqGXw@mail.gmail.com [4] https://www.postgresql.org/message-id/CA%2BTgmoa1DCQ0MdojD9o6Ppbfj%3DabXxe4FUkwA4O_6qBHwOMVjw%40mail.gmail.com [5] https://www.postgresql.org/message-id/CA+TgmoaiTNj4DgQy42OT9JmTTP1NWcMV+ke0i=+a7=VgnzqGXw@mail.gmail.com
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2025-04-03T10:19:53Z
On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <amitlangote09@gmail.com> 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. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2025-10-21T04:07:19Z
On Thu, Apr 3, 2025 at 7:19 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <amitlangote09@gmail.com> 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Pavel Stehule <pavel.stehule@gmail.com> — 2025-10-21T05:10:10Z
Hi út 21. 10. 2025 v 6:07 odesílatel Amit Langote <amitlangote09@gmail.com> napsal: > On Thu, Apr 3, 2025 at 7:19 PM Amit Langote <amitlangote09@gmail.com> > wrote: > > On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <amitlangote09@gmail.com> > 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. > This is very interesting and important feature - I can help with testing and review if it will be necessary Regards Pavel > > -- > Thanks, Amit Langote > > >
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2025-10-22T13:55:53Z
. On Tue, Oct 21, 2025 at 2:10 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > út 21. 10. 2025 v 6:07 odesílatel Amit Langote <amitlangote09@gmail.com> napsal: >> >> On Thu, Apr 3, 2025 at 7:19 PM Amit Langote <amitlangote09@gmail.com> wrote: >> > On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <amitlangote09@gmail.com> 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. > > > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2025-12-01T06:09:00Z
Hi, On Wed, Oct 22, 2025 at 9:56 PM Amit Langote <amitlangote09@gmail.com> wrote: > > . > On Tue, Oct 21, 2025 at 2:10 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > > út 21. 10. 2025 v 6:07 odesílatel Amit Langote <amitlangote09@gmail.com> napsal: > >> > >> On Thu, Apr 3, 2025 at 7:19 PM Amit Langote <amitlangote09@gmail.com> wrote: > >> > On Fri, Dec 20, 2024 at 1:23 PM Amit Langote <amitlangote09@gmail.com> 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. > > > > > > 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 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-02-19T09:21:27Z
Hi Junwang, On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > 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. 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): create table pk (a numeric primary key); create table fk (a bigint references pk); insert into pk select generate_series(1, 2000000); 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) 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) 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 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-02-23T13:45:39Z
Hi Amit, On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Junwang, > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > 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. > > 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): > > create table pk (a numeric primary key); > create table fk (a bigint references pk); > insert into pk select generate_series(1, 2000000); > 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-02-28T07:08:05Z
Hi Junwang, On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > I re-ran the benchmarks (same test as yours, different machine): > > > > create table pk (a numeric primary key); > > create table fk (a bigint references pk); > > insert into pk select generate_series(1, 2000000); > > 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 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. > 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); > } 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. > > 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 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. On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > Hi Amit, > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > Hi Junwang, > > > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > 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. > > > > 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): > > > > create table pk (a numeric primary key); > > create table fk (a bigint references pk); > > insert into pk select generate_series(1, 2000000); > > 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 -- Thanks, Amit Langote -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Tomas Vondra <tomas@vondra.me> — 2026-03-01T12:22:49Z
On 2/28/26 08:08, Amit Langote wrote: > Hi Junwang, > > ... > > 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. > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-02T07:49:05Z
Hi Tomas, Thanks for the thorough benchmarking. On Sun, Mar 1, 2026 at 9:22 PM Tomas Vondra <tomas@vondra.me> wrote: > On 2/28/26 08:08, Amit Langote wrote: > > 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. > > > > 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. 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? > 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 ... 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 > ---------- > > 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. 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. > 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. 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. > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Tomas Vondra <tomas@vondra.me> — 2026-03-02T12:34:41Z
On 3/2/26 08:49, Amit Langote wrote: > Hi Tomas, > > Thanks for the thorough benchmarking. > > On Sun, Mar 1, 2026 at 9:22 PM Tomas Vondra <tomas@vondra.me> wrote: >> On 2/28/26 08:08, Amit Langote wrote: >>> 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. >>> >> >> 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. > > 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? > All the changes were specific to the index prefetching patch, due to small changes in API (function name, new argument, ...). >> 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 ... > > 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. > I think it's mostly independent, I don't see much code overlap. The index insert prefetch needs to happen much earlier, not in the "after trigger" phase (which is where FK checks happen, right?). It also works with individual tuples by default - it prefetches leaf pages for all indexes that one insert needs to touch. There is some batching concept e.g. for COPY. Anyway, it's a separate patch. Let's not discuss that here, so that we don't distract people interested in the FK stuff. >> 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. > > 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. > Seems like it. >> 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. > > 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. > Right. I think the sequential case maximizes the amount of CPU time spent in the FK checks, so the reduction is much more visible. >> 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. > > 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.. > TBH I haven't noticed the memory context issue myself, I only noticed because the builds with index prefetch started crashing. But I'll try to take a look this week. regards -- Tomas Vondra
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-02T15:30:58Z
On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Junwang, > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > I re-ran the benchmarks (same test as yours, different machine): > > > > > > create table pk (a numeric primary key); > > > create table fk (a bigint references pk); > > > insert into pk select generate_series(1, 2000000); > > > 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 > > 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. > > > 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); > > } > > 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. nice improvement. > 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. > > > > 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 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. impressive! > > 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 Yeah, this is much better than the fpmeta_valid field. > 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. > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > Hi Amit, > > > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > Hi Junwang, > > > > > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > 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. > > > > > > 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): > > > > > > create table pk (a numeric primary key); > > > create table fk (a bigint references pk); > > > insert into pk select generate_series(1, 2000000); > > > 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 > > > > -- > Thanks, Amit Langote -- Regards Junwang Zhao -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-10T12:28:47Z
Hi, On Mon, Mar 2, 2026 at 11:30 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > Hi Junwang, > > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > I re-ran the benchmarks (same test as yours, different machine): > > > > > > > > create table pk (a numeric primary key); > > > > create table fk (a bigint references pk); > > > > insert into pk select generate_series(1, 2000000); > > > > 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 > > > > 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. > > > > > 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); > > > } > > > > 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. > > nice improvement. > > > 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. > > > > > > 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 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. > > impressive! > > > > > 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 > > Yeah, this is much better than the fpmeta_valid field. > > > 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. > > > > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > > > Hi Amit, > > > > > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > > > Hi Junwang, > > > > > > > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > > 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. > > > > > > > > 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): > > > > > > > > create table pk (a numeric primary key); > > > > create table fk (a bigint references pk); > > > > insert into pk select generate_series(1, 2000000); > > > > 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 > > > > > > > > -- > > Thanks, Amit Langote > > > > -- > Regards > Junwang Zhao 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-16T14:03:41Z
Hi Junwang, Thanks for sending the new version. On Tue, Mar 10, 2026 at ... Junwang Zhao <zhjwpku@gmail.com> wrote: > 1. > Move ri_ReportViolation into ri_FastPathCheck, so table_open is no > longer needed, and ri_FastPathCheck now returns void. Good, kept. > 2. > After adding the batch fast path, the original ri_FastPathCheck is only > used by the ALTER TABLE validation path. This path cannot use the > cache because the registered AfterTriggerBatch callback will never run. > Therefore, the use_cache branch can be removed. Agreed. I went a step further and restructured 0002 to avoid the use_cache branching entirely. Instead of adding if/else blocks to ri_FastPathCheck, 0002 now adds a separate ri_FastPathCheckCached() function with its own resource lifecycle. 0003 then replaces it with ri_FastPathBatchAdd() -- a clean swap rather than completely undoing what 0002 added. This also removes the use_cache parameter from ri_FastPathProbeOne; the memory context switch to TopTransactionContext is now the caller's responsibility. > 3. > ri_FastPathBatchFlush creates a new fk_slot but does not cache it in > RI_FastPathEntry. I tried caching it in v5-0006 and ran some benchmarks, > it didn't show much improvement. I put the fk_slot in the cache entry since it's a small change. > 4. > ri_FastPathFlushArray currently uses SK_SEARCHARRAY only for > single-column checks. [...] my current understanding is that > SK_SEARCHARRAY may not work for multi-column checks. Right, I haven't investigated this deeply either. The FlushLoop fallback is the right approach for now. If we want to explore a SEARCHARRAY approach for multi-column keys in a follow-up, it would be worth checking with Peter Geoghegan or someone else familiar with the btree SAOP internals on how multiple array keys across columns are iterated and whether that's usable at all for this use case. Attached is v6, three patches -- combined the old 0003 (buffering) and 0004 (SK_SEARCHARRAY) into a single 0003, since the buffering alone has no performance benefit (or at least only minor) and the split added unnecessary diff/rebase churn. The biggest change in this version is the snapshot handling. Looking more carefully at what the SPI path actually does for RI_FKey_check (non-partitioned PK, detectNewRows = false), I found that ri_PerformCheck passes InvalidSnapshot to SPI_execute_snapshot, and _SPI_execute_plan ends up doing PushActiveSnapshot(GetTransactionSnapshot()). So the SPI path scans with the transaction snapshot, not the latest snapshot. So I've changed the fast path to match: ri_FastPathCheck now uses GetTransactionSnapshot() directly. Under READ COMMITTED this is a fresh snapshot; under REPEATABLE READ it's the frozen transaction-start snapshot, so PK rows committed after the transaction started are simply not visible. This means the second index probe (the IsolationUsesXactSnapshot crosscheck block) is no longer needed and is removed. The existing fk-snapshot isolation test confirms this is the correct behavior. Other changes since v5: * Fixed the batch callback firing during nested SPI: another AFTER trigger doing DML via SPI would call AfterTriggerEndQuery at a nested level, tearing down our cache mid-batch. Fixed by checking query_depth inside FireAfterTriggerBatchCallbacks. Added a test case with a trigger that auto-provisions PK rows via SPI. * Security context is now restored before ri_ReportViolation, not after (the ereport doesn't return). * search_vals[] and matched[] moved from RI_FastPathEntry to stack locals in ri_FastPathFlushArray -- they're rewritten from scratch on every flush with no state carried between calls. * Various comment fixes. I think this is getting close to committable shape. That said, another pair of eyes would be reassuring before I pull the trigger. Tomas, if you've had a chance to look, would welcome your thoughts. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Haibo Yan <tristan.yim@gmail.com> — 2026-03-17T00:28:02Z
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. 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. 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(). 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. 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. Thanks again for working on this. Best regards, Haibo Yan > On Mar 10, 2026, at 5:28 AM, Junwang Zhao <zhjwpku@gmail.com> wrote: > > Hi, > > On Mon, Mar 2, 2026 at 11:30 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >> >> On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: >>> >>> Hi Junwang, >>> >>> On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >>>> On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: >>>>> I re-ran the benchmarks (same test as yours, different machine): >>>>> >>>>> create table pk (a numeric primary key); >>>>> create table fk (a bigint references pk); >>>>> insert into pk select generate_series(1, 2000000); >>>>> 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 >>> >>> 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. >>> >>>> 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); >>>> } >>> >>> 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. >> >> nice improvement. >> >>> 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. >>> >>>>> 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 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. >> >> impressive! >> >>> >>> 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 >> >> Yeah, this is much better than the fpmeta_valid field. >> >>> 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. >>> >>> >>> On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >>>> >>>> Hi Amit, >>>> >>>> On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: >>>>> >>>>> Hi Junwang, >>>>> >>>>> On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >>>>>> 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. >>>>> >>>>> 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): >>>>> >>>>> create table pk (a numeric primary key); >>>>> create table fk (a bigint references pk); >>>>> insert into pk select generate_series(1, 2000000); >>>>> 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 >>> >>> >>> >>> -- >>> Thanks, Amit Langote >> >> >> >> -- >> Regards >> Junwang Zhao > > 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 > <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> -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-18T15:34:30Z
Hi Haibo, On Tue, Mar 17, 2026 at 8:28 AM Haibo Yan <tristan.yim@gmail.com> 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 <zhjwpku@gmail.com> wrote: > > Hi, > > On Mon, Mar 2, 2026 at 11:30 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > Hi Junwang, > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > I re-ran the benchmarks (same test as yours, different machine): > > create table pk (a numeric primary key); > create table fk (a bigint references pk); > insert into pk select generate_series(1, 2000000); > 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 > > > 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. > > 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); > } > > > 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. > > > nice improvement. > > 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. > > 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 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. > > > impressive! > > > 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 > > > Yeah, this is much better than the fpmeta_valid field. > > 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. > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > Hi Amit, > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > Hi Junwang, > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > 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. > > > 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): > > create table pk (a numeric primary key); > create table fk (a bigint references pk); > insert into pk select generate_series(1, 2000000); > 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 > > > > > -- > Thanks, Amit Langote > > > > > -- > Regards > Junwang Zhao > > > 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 > <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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-19T16:19:53Z
On Wed, Mar 18, 2026 at 11:34 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > Hi Haibo, > > On Tue, Mar 17, 2026 at 8:28 AM Haibo Yan <tristan.yim@gmail.com> 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 <zhjwpku@gmail.com> wrote: > > > > Hi, > > > > On Mon, Mar 2, 2026 at 11:30 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > > > On Sat, Feb 28, 2026 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > Hi Junwang, > > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > I re-ran the benchmarks (same test as yours, different machine): > > > > create table pk (a numeric primary key); > > create table fk (a bigint references pk); > > insert into pk select generate_series(1, 2000000); > > 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 > > > > > > 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. > > > > 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); > > } > > > > > > 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. > > > > > > nice improvement. > > > > 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. > > > > 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 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. > > > > > > impressive! > > > > > > 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 > > > > > > Yeah, this is much better than the fpmeta_valid field. > > > > 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. > > > > > > On Mon, Feb 23, 2026 at 10:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > > > Hi Amit, > > > > On Thu, Feb 19, 2026 at 5:21 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > Hi Junwang, > > > > On Mon, Dec 1, 2025 at 3:09 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > 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. > > > > > > 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): > > > > create table pk (a numeric primary key); > > create table fk (a bigint references pk); > > insert into pk select generate_series(1, 2000000); > > 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 > > > > > > > > > > -- > > Thanks, Amit Langote > > > > > > > > > > -- > > Regards > > Junwang Zhao > > > > > > 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 > > <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 I squashed 0004 into 0003 so that each file can be committed independently. I also runned pgindent for each file. -- Regards Junwang Zhao -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-20T08:20:04Z
On Mon, Mar 16, 2026 at 23:03 Amit Langote <amitlangote09@gmail.com> wrote: > Hi Junwang, > > Thanks for sending the new version. > > On Tue, Mar 10, 2026 at ... Junwang Zhao <zhjwpku@gmail.com> wrote: > > 1. > > Move ri_ReportViolation into ri_FastPathCheck, so table_open is no > > longer needed, and ri_FastPathCheck now returns void. > > Good, kept. > > > 2. > > After adding the batch fast path, the original ri_FastPathCheck is only > > used by the ALTER TABLE validation path. This path cannot use the > > cache because the registered AfterTriggerBatch callback will never run. > > Therefore, the use_cache branch can be removed. > > Agreed. I went a step further and restructured 0002 to avoid the > use_cache branching entirely. Instead of adding if/else blocks to > ri_FastPathCheck, 0002 now adds a separate ri_FastPathCheckCached() > function with its own resource lifecycle. 0003 then replaces it with > ri_FastPathBatchAdd() -- a clean swap rather than completely undoing > what 0002 added. This also removes the use_cache parameter from > ri_FastPathProbeOne; the memory context switch to > TopTransactionContext is now the caller's responsibility. > > > 3. > > ri_FastPathBatchFlush creates a new fk_slot but does not cache it in > > RI_FastPathEntry. I tried caching it in v5-0006 and ran some benchmarks, > > it didn't show much improvement. > > I put the fk_slot in the cache entry since it's a small change. > > > 4. > > ri_FastPathFlushArray currently uses SK_SEARCHARRAY only for > > single-column checks. [...] my current understanding is that > > SK_SEARCHARRAY may not work for multi-column checks. > > Right, I haven't investigated this deeply either. The FlushLoop > fallback is the right approach for now. If we want to explore a > SEARCHARRAY approach for multi-column keys in a follow-up, it would be > worth checking with Peter Geoghegan or someone else familiar with the > btree SAOP internals on how multiple array keys across columns > are iterated and whether that's usable at all for this use case. > > Attached is v6, three patches -- combined the old 0003 (buffering) and > 0004 (SK_SEARCHARRAY) into a single 0003, since the buffering alone > has no performance benefit (or at least only minor) and the split > added unnecessary diff/rebase churn. > > The biggest change in this version is the snapshot handling. Looking > more carefully at what the SPI path actually does for RI_FKey_check > (non-partitioned PK, detectNewRows = false), I found that > ri_PerformCheck passes InvalidSnapshot to SPI_execute_snapshot, and > _SPI_execute_plan ends up doing > PushActiveSnapshot(GetTransactionSnapshot()). So the SPI path scans > with the transaction snapshot, not the latest > snapshot. > > So I've changed the fast path to match: ri_FastPathCheck now uses > GetTransactionSnapshot() directly. Under READ COMMITTED this is a > fresh snapshot; under REPEATABLE READ it's the frozen > transaction-start snapshot, so PK rows committed after the transaction > started are simply not visible. This means the second index probe > (the IsolationUsesXactSnapshot crosscheck block) is no longer needed > and is removed. The existing fk-snapshot isolation test confirms this > is the correct behavior. > > Other changes since v5: > > * Fixed the batch callback firing during nested SPI: another AFTER > trigger doing DML via SPI would call AfterTriggerEndQuery at a nested > level, tearing down our cache mid-batch. Fixed by checking > query_depth inside FireAfterTriggerBatchCallbacks. Added a test case > with a trigger that auto-provisions PK rows via SPI. > > * Security context is now restored before ri_ReportViolation, not > after (the ereport doesn't return). > > * search_vals[] and matched[] moved from RI_FastPathEntry to stack > locals in ri_FastPathFlushArray -- they're rewritten from scratch on > every flush with no state carried between calls. > > * Various comment fixes. > > I think this is getting close to committable shape. That said, another > pair of eyes would be reassuring before I pull the trigger. Tomas, if > you've had a chance to look, would welcome your thoughts. Adding Tomas to cc -- I thought he was already on the thread when I sent this. Tomas, context is in the quoted email below. Junwang has posted a couple of updated versions since then addressing review feedback from Haibo Yan (memory context handling for the flush path, batch size rationale). Latest patches are in the thread. If you have time, would appreciate a look. Thanks, Amit >
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-24T11:47:17Z
Hi Junwang, On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > I squashed 0004 into 0003 so that each file can be committed independently. > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-24T13:56:18Z
On Tue, Mar 24, 2026 at 8:47 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Junwang, > > On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > I squashed 0004 into 0003 so that each file can be committed independently. > > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-25T00:41:11Z
On Tue, Mar 24, 2026 at 10:56 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Mar 24, 2026 at 8:47 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > Hi Junwang, > > > > On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > I squashed 0004 into 0003 so that each file can be committed independently. > > > 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. > > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-30T04:55:07Z
On Wed, Mar 25, 2026 at 9:41 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Mar 24, 2026 at 10:56 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Mar 24, 2026 at 8:47 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > Hi Junwang, > > > > > > On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > I squashed 0004 into 0003 so that each file can be committed independently. > > > > 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. > > > > 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. > > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-30T11:15:05Z
On Mon, Mar 30, 2026 at 1:55 PM Amit Langote <amitlangote09@gmail.com> wrote: > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-03-31T09:09:19Z
> On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com> wrote: > > On Mon, Mar 30, 2026 at 1:55 PM Amit Langote <amitlangote09@gmail.com> wrote: >> 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. > > 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 > <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/
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-31T09:17:16Z
Hi, On Tue, Mar 31, 2026 at 6:09 PM Chao Li <li.evan.chao@gmail.com> wrote: > > On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com> wrote: > > > > On Mon, Mar 30, 2026 at 1:55 PM Amit Langote <amitlangote09@gmail.com> wrote: > >> 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. > > > > 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 > > <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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-31T10:57:46Z
On Tue, Mar 31, 2026 at 5:17 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > On Tue, Mar 31, 2026 at 6:09 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > On Mon, Mar 30, 2026 at 1:55 PM Amit Langote <amitlangote09@gmail.com> wrote: > > >> 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. > > > > > > 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 > > > <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. > > 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 prion is happy now, the fix works, thanks. -- Regards Junwang Zhao
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Daniel Gustafsson <daniel@yesql.se> — 2026-03-31T11:22:37Z
> On 31 Mar 2026, at 12:57, Junwang Zhao <zhjwpku@gmail.com> wrote: > prion is happy now, the fix works, thanks. The widowbird failure seems to be SPI related as well, relevant portion of log below. Is that the same or another error? 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: -- Daniel Gustafsson
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-31T11:26:35Z
On Tue, Mar 31, 2026 at 8:22 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 31 Mar 2026, at 12:57, Junwang Zhao <zhjwpku@gmail.com> wrote: > > > prion is happy now, the fix works, thanks. > > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Daniel Gustafsson <daniel@yesql.se> — 2026-03-31T11:35:33Z
> On 31 Mar 2026, at 13:26, Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Mar 31, 2026 at 8:22 PM Daniel Gustafsson <daniel@yesql.se> wrote: >>> On 31 Mar 2026, at 12:57, Junwang Zhao <zhjwpku@gmail.com> wrote: >> >>> prion is happy now, the fix works, thanks. >> >> 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-03-31T12:15:22Z
On Tue, Mar 31, 2026 at 7:57 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > On Tue, Mar 31, 2026 at 5:17 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Mar 31, 2026 at 6:09 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > > On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com> wrote: > > > > 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 > > > > <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. > > > > 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 > > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Tomas Vondra <tomas@vondra.me> — 2026-03-31T13:33:53Z
On 3/31/26 13:35, Daniel Gustafsson wrote: >> On 31 Mar 2026, at 13:26, Amit Langote <amitlangote09@gmail.com> wrote: >> >> On Tue, Mar 31, 2026 at 8:22 PM Daniel Gustafsson <daniel@yesql.se> wrote: >>>> On 31 Mar 2026, at 12:57, Junwang Zhao <zhjwpku@gmail.com> wrote: >>> >>>> prion is happy now, the fix works, thanks. >>> >>> 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. >> 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-03-31T15:54:29Z
Hi Amit, On Tue, Mar 31, 2026 at 8:15 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Mar 31, 2026 at 7:57 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Tue, Mar 31, 2026 at 5:17 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > On Tue, Mar 31, 2026 at 6:09 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > > > On Mar 30, 2026, at 19:15, Amit Langote <amitlangote09@gmail.com> wrote: > > > > > 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 > > > > > <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. > > > > > > 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 > > > > 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. + 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. > > -- > Thanks, Amit Langote -- Regards Junwang Zhao -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-01T00:06:49Z
On Tue, Mar 31, 2026 at 10:33 PM Tomas Vondra <tomas@vondra.me> wrote: > On 3/31/26 13:35, Daniel Gustafsson wrote: > >> On 31 Mar 2026, at 13:26, Amit Langote <amitlangote09@gmail.com> wrote: > >> > >> On Tue, Mar 31, 2026 at 8:22 PM Daniel Gustafsson <daniel@yesql.se> wrote: > >>>> On 31 Mar 2026, at 12:57, Junwang Zhao <zhjwpku@gmail.com> wrote: > >>> > >>>> prion is happy now, the fix works, thanks. > >>> > >>> 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. > >> > > 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). I see, thanks for checking that. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-01T08:51:21Z
On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > + 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. Thanks for checking. I have just pushed a slightly modified version of that. > > 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. 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-01T09:51:10Z
On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > + 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. > > Thanks for checking. > > I have just pushed a slightly modified version of that. > > > > 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. > > 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. Pushed. > 0002 is rebased over that. Rebased again. -- Thanks, Amit Langote -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Junwang Zhao <zhjwpku@gmail.com> — 2026-04-01T11:56:10Z
On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > + 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. > > > > Thanks for checking. > > > > I have just pushed a slightly modified version of that. > > > > > > 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. > > > > 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. > > Pushed. > > > 0002 is rebased over that. > > Rebased again. +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. > > -- > Thanks, Amit Langote -- Regards Junwang Zhao -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-01T12:18:14Z
On Wed, Apr 1, 2026 at 8:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > + 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. > > > > > > Thanks for checking. > > > > > > I have just pushed a slightly modified version of that. > > > > > > > > 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. > > > > > > 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. > > > > Pushed. > > > > > 0002 is rebased over that. > > > > Rebased again. > > +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? 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. > 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. 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-02T07:41:30Z
On Wed, Apr 1, 2026 at 9:18 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Apr 1, 2026 at 8:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: > > > > > + 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. > > > > > > > > Thanks for checking. > > > > > > > > I have just pushed a slightly modified version of that. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > Pushed. > > > > > > > 0002 is rebased over that. > > > > > > Rebased again. > > > > +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? > > 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. > > > 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. > > 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. 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-02T07:59:37Z
> On Apr 2, 2026, at 15:41, Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Apr 1, 2026 at 9:18 PM Amit Langote <amitlangote09@gmail.com> wrote: >> On Wed, Apr 1, 2026 at 8:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote: >>> On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: >>>> >>>> On Wed, Apr 1, 2026 at 5:51 PM Amit Langote <amitlangote09@gmail.com> wrote: >>>>> On Wed, Apr 1, 2026 at 12:54 AM Junwang Zhao <zhjwpku@gmail.com> wrote: >>>>>> + 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. >>>>> >>>>> Thanks for checking. >>>>> >>>>> I have just pushed a slightly modified version of that. >>>>> >>>>>>> 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. >>>>> >>>>> 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. >>>> >>>> Pushed. >>>> >>>>> 0002 is rebased over that. >>>> >>>> Rebased again. >>> >>> +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? >> >> 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. >> >>> 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. >> >> 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. > > 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 > <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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-03T05:52:45Z
Hi, On Thu, Apr 2, 2026 at 5:00 PM Chao Li <li.evan.chao@gmail.com> wrote: > > On Apr 2, 2026, at 15:41, Amit Langote <amitlangote09@gmail.com> wrote: > > Will push tomorrow morning (Friday) barring objections. > > <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”. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-03T08:57:36Z
> On Apr 3, 2026, at 13:52, Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > On Thu, Apr 2, 2026 at 5:00 PM Chao Li <li.evan.chao@gmail.com> wrote: >>> On Apr 2, 2026, at 15:41, Amit Langote <amitlangote09@gmail.com> wrote: >>> Will push tomorrow morning (Friday) barring objections. >>> <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”. > > 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 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-03T09:39:48Z
On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: > > On Apr 3, 2026, at 13:52, Amit Langote <amitlangote09@gmail.com> wrote: > > On Thu, Apr 2, 2026 at 5:00 PM Chao Li <li.evan.chao@gmail.com> wrote: > >> 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. > > 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. 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-06T09:45:05Z
On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Apr 3, 2026, at 13:52, Amit Langote <amitlangote09@gmail.com> wrote: > > > On Thu, Apr 2, 2026 at 5:00 PM Chao Li <li.evan.chao@gmail.com> wrote: > > >> 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. > > > > 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. > > 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. 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-07T01:45:42Z
> On Apr 6, 2026, at 17:45, Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <amitlangote09@gmail.com> wrote: >> On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: >>>> On Apr 3, 2026, at 13:52, Amit Langote <amitlangote09@gmail.com> wrote: >>>> On Thu, Apr 2, 2026 at 5:00 PM Chao Li <li.evan.chao@gmail.com> wrote: >>>>> 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. >>> >>> 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. >> >> 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. > > 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 > <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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-07T02:12:15Z
On Tue, Apr 7, 2026 at 10:46 AM Chao Li <li.evan.chao@gmail.com> wrote: > > On Apr 6, 2026, at 17:45, Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > >> On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: > >>> 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. > >> > >> 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. > > > > 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 > > 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". > ``` Thanks for checking. Pushed. -- Thanks, Amit Langote -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Evan Montgomery-Recht <montge@mianetworks.net> — 2026-04-07T12:59:59Z
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 On Mon, Apr 6, 2026 at 10:12 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Apr 7, 2026 at 10:46 AM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Apr 6, 2026, at 17:45, Amit Langote <amitlangote09@gmail.com> wrote: > > > On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > > >> On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li.evan.chao@gmail.com> wrote: > > >>> 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. > > >> > > >> 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. > > > > > > 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 > > > > 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". > > ``` > > Thanks for checking. Pushed. > > -- > Thanks, Amit Langote > > -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-08T01:23:59Z
Hi Evan, On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht <montge@mianetworks.net> wrote: > > 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). 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 > 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. Makes sense. Will push the attached patch for this. -- Thanks, Amit Langote -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-08T09:58:23Z
On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > <montge@mianetworks.net> wrote: > > 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). 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-08T14:26:21Z
On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > > <montge@mianetworks.net> wrote: > > > 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). > > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-09T07:39:44Z
> On Apr 8, 2026, at 22:26, Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <amitlangote09@gmail.com> wrote: >> On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: >>> On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht >>> <montge@mianetworks.net> wrote: >>>> 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). >> >> 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. > > 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 > <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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-09T08:40:49Z
Hi, On Thu, Apr 9, 2026 at 4:40 PM Chao Li <li.evan.chao@gmail.com> wrote: > > On Apr 8, 2026, at 22:26, Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <amitlangote09@gmail.com> wrote: > >> On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > >>> On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > >>> <montge@mianetworks.net> wrote: > >>>> 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). > >> > >> 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. > > > > 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. > A few comments on v3: Thanks for the review. > 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. I'm dropping 0002 or have merged 0004 into it so this memory context switch is no longer present. > 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? 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
jie wang <jugierwang@gmail.com> — 2026-04-09T09:21:51Z
Amit Langote <amitlangote09@gmail.com> 于2026年4月9日周四 16:41写道: > Hi, > > On Thu, Apr 9, 2026 at 4:40 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Apr 8, 2026, at 22:26, Amit Langote <amitlangote09@gmail.com> > wrote: > > > On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <amitlangote09@gmail.com> > wrote: > > >> On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> > wrote: > > >>> On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > > >>> <montge@mianetworks.net> wrote: > > >>>> 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). > > >> > > >> 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. > > > > > > 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. > > A few comments on v3: > > Thanks for the review. > > > 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. > > I'm dropping 0002 or have merged 0004 into it so this memory context > switch is no longer present. > > > 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? > > 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 > 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 -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-09T09:24:47Z
On Thu, Apr 9, 2026 at 6:22 PM jie wang <jugierwang@gmail.com> wrote: > 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. Thanks, I've fixed the typo in my local tree. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-09T09:29:04Z
On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > <montge@mianetworks.net> wrote: > > 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. > > Makes sense. Will push the attached patch for this. Pushed this fix. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-09T10:25:21Z
> On Apr 9, 2026, at 16:40, Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > On Thu, Apr 9, 2026 at 4:40 PM Chao Li <li.evan.chao@gmail.com> wrote: >>> On Apr 8, 2026, at 22:26, Amit Langote <amitlangote09@gmail.com> wrote: >>> On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <amitlangote09@gmail.com> wrote: >>>> On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: >>>>> On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht >>>>> <montge@mianetworks.net> wrote: >>>>>> 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). >>>> >>>> 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. >>> >>> 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. >> A few comments on v3: > > Thanks for the review. > >> 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. > > I'm dropping 0002 or have merged 0004 into it so this memory context > switch is no longer present. > >> 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? > > 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 > <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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Eliminating SPI / SQL from some RI triggers - take 3
Sandro Santilli <strk@kbt.io> — 2026-04-09T11:07:50Z
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;
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-09T11:55:01Z
Hi Sandro, On Thu, Apr 9, 2026 at 8:07 PM Sandro Santilli <strk@kbt.io> wrote: > 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. 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Sandro Santilli <strk@kbt.io> — 2026-04-09T16:01:58Z
On Thu, Apr 09, 2026 at 08:55:01PM +0900, Amit Langote wrote: > Hi Sandro, > > On Thu, Apr 9, 2026 at 8:07 PM Sandro Santilli <strk@kbt.io> wrote: > > 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. > > Evan Montgomery-Recht posted a report of the same issue on this thread > a couple of days ago. 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 > I have posted a patch to fix the issue, which I will commit tomorrow > 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;
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-10T04:14:11Z
Hi Sandro, On Fri, Apr 10, 2026 at 1:02 AM Sandro Santilli <strk@kbt.io> wrote: > On Thu, Apr 09, 2026 at 08:55:01PM +0900, Amit Langote wrote: > > Hi Sandro, > > > > On Thu, Apr 9, 2026 at 8:07 PM Sandro Santilli <strk@kbt.io> wrote: > > > 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. > > > > Evan Montgomery-Recht posted a report of the same issue on this thread > > a couple of days ago. > > 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 > > > I have posted a patch to fix the issue, which I will commit tomorrow > > 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 ! 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Chao Li <li.evan.chao@gmail.com> — 2026-04-10T04:20:24Z
> On Apr 10, 2026, at 12:14, Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Sandro, > > On Fri, Apr 10, 2026 at 1:02 AM Sandro Santilli <strk@kbt.io> wrote: >> On Thu, Apr 09, 2026 at 08:55:01PM +0900, Amit Langote wrote: >>> Hi Sandro, >>> >>> On Thu, Apr 9, 2026 at 8:07 PM Sandro Santilli <strk@kbt.io> wrote: >>>> 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. >>> >>> Evan Montgomery-Recht posted a report of the same issue on this thread >>> a couple of days ago. >> >> 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 >> >>> I have posted a patch to fix the issue, which I will commit tomorrow >>> 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 ! > > 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 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, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-10T04:21:37Z
On Fri, Apr 10, 2026 at 1:21 PM Chao Li <li.evan.chao@gmail.com> wrote: > > On Apr 10, 2026, at 12:14, Amit Langote <amitlangote09@gmail.com> wrote: > > > > Hi Sandro, > > > > On Fri, Apr 10, 2026 at 1:02 AM Sandro Santilli <strk@kbt.io> wrote: > >> On Thu, Apr 09, 2026 at 08:55:01PM +0900, Amit Langote wrote: > >>> Hi Sandro, > >>> > >>> On Thu, Apr 9, 2026 at 8:07 PM Sandro Santilli <strk@kbt.io> wrote: > >>>> 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. > >>> > >>> Evan Montgomery-Recht posted a report of the same issue on this thread > >>> a couple of days ago. > >> > >> 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 > >> > >>> I have posted a patch to fix the issue, which I will commit tomorrow > >>> 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 ! > > > > 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 > > 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. Yep, my bad. Will push a fix shortly. -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-10T07:39:16Z
On Thu, Apr 9, 2026 at 7:26 PM Chao Li <li.evan.chao@gmail.com> wrote: > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Sandro Santilli <strk@kbt.io> — 2026-04-10T18:35:36Z
On Fri, Apr 10, 2026 at 01:14:11PM +0900, Amit Langote wrote: > > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Haibo Yan <tristan.yim@gmail.com> — 2026-04-10T23:34:21Z
On Fri, Apr 10, 2026 at 12:39 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Thu, Apr 9, 2026 at 7:26 PM Chao Li <li.evan.chao@gmail.com> wrote: > > 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 > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-15T06:03:50Z
On Sat, Apr 11, 2026 at 8:34 AM Haibo Yan <tristan.yim@gmail.com> wrote: > On Fri, Apr 10, 2026 at 12:39 AM Amit Langote <amitlangote09@gmail.com> wrote: >> 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. > > I reviewed the patch, and overall it looks close. I have a few comments: > > 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. > > 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. > > 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
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
Peter Eisentraut <peter@eisentraut.org> — 2026-04-20T20:50:29Z
On 02.04.26 09:41, Amit Langote wrote: > 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. 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. -
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-21T00:52:55Z
On Tue, Apr 21, 2026 at 5:50 AM Peter Eisentraut <peter@eisentraut.org> wrote: > On 02.04.26 09:41, Amit Langote wrote: > > 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. > > 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. Thanks for the patch. LGTM. Are you planning to push it or do you want me to? -- Thanks, Amit Langote
-
Re: Eliminating SPI / SQL from some RI triggers - take 3
amit <amitlangote09@gmail.com> — 2026-04-22T04:04:17Z
On Tue, Apr 21, 2026 at 9:52 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Apr 21, 2026 at 5:50 AM Peter Eisentraut <peter@eisentraut.org> wrote: > > On 02.04.26 09:41, Amit Langote wrote: > > > 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. > > > > 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. > > Thanks for the patch. LGTM. > > Are you planning to push it or do you want me to? Pushed. -- Thanks, Amit Langote