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

On Tue, Mar 24, 2026 at 10:56 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Mar 24, 2026 at 8:47 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > Hi Junwang,
> >
> > On Fri, Mar 20, 2026 at 1:20 AM Junwang Zhao <zhjwpku(at)gmail(dot)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