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

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