On Wed, Mar 25, 2026 at 9:41 AM Amit Langote <[email protected]> wrote: > On Tue, Mar 24, 2026 at 10:56 PM Amit Langote <[email protected]> wrote: > > On Tue, Mar 24, 2026 at 8:47 PM Amit Langote <[email protected]> > > wrote: > > > > > > Hi Junwang, > > > > > > On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <[email protected]> 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
v11-0002-Batch-FK-rows-and-use-SK_SEARCHARRAY-for-fast-pa.patch
Description: Binary data
v11-0001-Add-fast-path-for-foreign-key-constraint-checks.patch
Description: Binary data
