Re: Question: Should we release the FK fast-path pk_slot's buffer pin promptly? - Mailing list pgsql-hackers

Hi Amul,

On Wed, May 6, 2026 at 6:53 PM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi all,
>
> While debugging an issue in AfterTriggerEndQuery [1], I wondered about
> the batched FK fast-path code added by commit b7b27eb41a5: should
> ri_FastPathBatchFlush release pk_slot's buffer pin immediately after
> index_endscan, instead of letting it live until either the next batch's
> first probe or end-of-statement teardown?
>
> My initial thought was YES -- a stale pin sitting around looks wrong
> -- but the more I look at it, the more skeptical I become.
>
> Currently, RI_FastPathEntry caches pk_rel, idx_rel, pk_slot,
> fk_slot, and a per-batch scratch MemoryContext.  The cache lives until
> AfterTriggerEndQuery, when ri_FastPathEndBatch flushes the remaining
> buffered rows and ri_FastPathTeardown drops everything.
>
> Inside ri_FastPathBatchFlush, index_getnext_slot stores each matched PK
> heap tuple into pk_slot.  Because pk_slot is a buffer-heap-tuple slot,
> that pins the buffer and registers the pin on CurrentResourceOwner.
> After index_endscan() returns, the slot still holds the last-probed
> tuple; the pin stays live until either:
>
> 1) the next probe's index_getnext_slot clears the slot before storing
> a new tuple, or
>
> 2) end-of-statement teardown drops the slot, releasing the pin.
>
> So the pin survives for a while but eventually gets cleaned up.
> Question is whether the gap is worth tightening with an explicit
> ExecClearTuple(fpentry->pk_slot) right after index_endscan.
>
> Reasons to favor the explicit clear:
>
> a) Buffer eviction.  A pinned buffer is unevictable.  Nothing in
> ri_triggers.c references pk_slot again before teardown, so the pin is
> functionally orphaned and slightly reduces the effective working set
> for the remainder of the statement.
>
> b)  Pin-lifetime discipline. ExecStoreBufferHeapTuple registers the
> pin on CurrentResourceOwner; ExecDropSingleTupleTableSlot in teardown
> asks the current CurrentResourceOwner to forget it. Today they
> coincide, but the gap between probe and teardown spans arbitrary user
> code. Clearing the slot right after the probe closes that gap for any
> future or out-of-tree caller (extensions or forks) that ends up
> invoking ri_FastPathTeardown from a different ResourceOwner context.

Yeah, there is a gap between ri_FastPathBatchAdd and ri_FastPathBatchFlush,
ri_FastPathBatchFlush is holding the pin of a not likely using pk slot.
Your changes after index_endscan makes sense to me.

>
> c) pinning_backends.  VACUUM truncation and a few buffer-related
> operations slow down or skip work when buffers are pinned. Holding a
> fast-path pin across unrelated execution burns that budget.
>
> But none of those seem strictly concerning for today. However, the fix
> is not problematic or very complicated and seems harmless to include
> to address these trivial concerns; it would have two ExecClearTuple()
> calls: one in ri_FastPathBatchFlush after index_endscan, and a
> defensive pre-clear in ri_FastPathTeardown. Patch attached.

I have no idea whether the change in ri_FastPathTeardown is necessary,
I'm not sure what the error path in the comments can be.

+ * (e.g., during an error path). This avoids relying on
+ * ExecDropSingleTupleTableSlot to handle the release implicitly. fk_slot
+ * does not need this treatment as it uses TTSOpsHeapTuple and never pins
+ * buffers.

I don't get this, ExecDropSingleTupleTableSlot is called in a few lines below,
and ExecDropSingleTupleTableSlot calls ExecClearTuple, what's the point
of calling *ExecClearTuple(entry->pk_slot);* explicitly?


>
> Thoughts?
>
> 1] http://postgr.es/m/CAAJ_b95p6-qiVpE2Gpr=bUsNAqTcejD_rPgLnfjx9m=fo3Rf3Q@mail.gmail.com
>
> --
> Regards,
> Amul Sul
> EDB: http://www.enterprisedb.com



--
Regards
Junwang Zhao