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

On Thu, May 7, 2026 at 8:17 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> 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?

Thanks for taking a look. You're right -- the call is redundant in the
success path, and even in the error path, the implicit clear inside
ExecDropSingleTupleTableSlot would release the pin under the same
CurrentResourceOwner that the explicit clear would use, since both run
inside the same teardown call. In the postgres code tree, the owner
that took the pin (in ri_FastPathProbeOne) and the owner active at
teardown are the same, so there's no mismatch either way.

I added it as an extra precaution against a hypothetical future error
path that bypasses the flush-time clear, but given the above, it
doesn't change which owner the pin is released against, so it's
essentially cosmetic. Happy to drop it.

Regards,
Amul