Technical Analysis: Releasing FK Fast-Path pk_slot Buffer Pin Promptly

Architectural Context

Commit b7b27eb41a5 introduced a batched fast-path mechanism for foreign key referential integrity (RI) checking in src/backend/utils/adt/ri_triggers.c. The traditional RI trigger path executes a parameterized SPI query per row to verify that a referenced PK row exists; the fast-path instead caches open relations, index-scan state, and per-batch slots across many tuples in the same statement, amortizing planning/SPI overhead. The cache is materialized as an RI_FastPathEntry holding pk_rel, idx_rel, pk_slot (a BufferHeapTupleSlot), fk_slot, and a per-batch MemoryContext. Its lifecycle is bounded by AfterTriggerEndQuery → ri_FastPathEndBatch → ri_FastPathTeardown.

The specific mechanic under scrutiny: inside ri_FastPathBatchFlush, each PK probe is executed through index_getnext_slot, which calls ExecStoreBufferHeapTuple on pk_slot. That call pins the heap buffer holding the matched tuple and registers the pin with CurrentResourceOwner. After index_endscan returns, the slot still materializes the last-returned tuple and thus still holds a buffer pin. This pin is only implicitly released when either:

1. The next probe's index_getnext_slot re-stores into the slot (which clears the prior pin), or
2. End-of-statement teardown drops the single-tuple slot via ExecDropSingleTupleTableSlot, which releases the pin through the resource owner.

The Core Question

Sulamul (the reporter) asks whether ri_FastPathBatchFlush should eagerly clear pk_slot (via ExecClearTuple) immediately after index_endscan, rather than letting the pin linger until the next batch or statement end. The ambivalence in the post is notable: the author starts leaning "yes" (stale pins smell wrong) and concludes "not strictly concerning, but harmless to tighten."

Arguments in Favor of the Explicit Clear

Three justifications are articulated, each targeting a different subsystem:

1. Buffer manager working-set pressure. A pinned buffer cannot be evicted by the clock-sweep replacement algorithm (StrategyGetBuffer skips buffers with refcount > 0). Holding a pin on a PK heap page between the end of a batch and statement teardown — during which arbitrary user code and other executor activity may run — reduces the pool of evictable buffers by one per active FK fast-path entry. On workloads with tight shared_buffers this is measurable, though on modern systems it is negligible.

2. ResourceOwner discipline. This is the most architecturally interesting point. ExecStoreBufferHeapTuple registers the pin on whatever CurrentResourceOwner is active at probe time; ExecDropSingleTupleTableSlot at teardown expects the then-current ResourceOwner to own the pin. Today these coincide because FK triggers fire within a stable ResourceOwner context, but the gap "spans arbitrary user code." If an extension, a custom trigger, or a fork re-enters executor code that switches ResourceOwner (e.g., a subtransaction with its own owner, a background portal, or a PL handler that pushes a new owner), the pin can become owned by one ResourceOwner and released against another — a classic source of ResourceOwnerForget assertion failures or leaked pins at commit. Clearing the slot immediately after the scan closes this window entirely.

3. pinning_backends / VACUUM interaction. lazy_scan_heap and heap truncation in lazy_truncate_heap back off when BufferIsPinned on relevant pages; ConditionalLockBufferForCleanup fails if any other backend holds a pin. A stray fast-path pin on a PK page held across the remainder of a long statement could delay VACUUM's ability to acquire a cleanup lock or to truncate. Again, the probability is low but non-zero.

Why the Author Remains Skeptical

The author explicitly notes "none of those seem strictly concerning for today." The current code is correct: the ResourceOwner is stable, the pin will be released, and there is no leak. The proposed patch is defensive hardening, not a bug fix. This distinction matters for review: PostgreSQL committers generally require a concrete motivating scenario for such changes, especially in performance-sensitive RI code.

The Proposed Patch

Two ExecClearTuple call sites:

• In ri_FastPathBatchFlush, immediately after index_endscan: releases the pin at the earliest correct moment — once the scan is closed, the slot's contents are definitionally no longer needed for this batch.
• In ri_FastPathTeardown, as a defensive pre-clear before ExecDropSingleTupleTableSlot: ensures the pin (if any) is released under the current ResourceOwner before the slot-drop path touches it. This is belt-and-suspenders for point (2) above.

The patch is small, localized, and has no semantic effect on correctness in the current tree — it only shortens pin lifetime.

Design Tradeoffs and Likely Review Reception

The case for the change is weakest on performance (no measurable win expected) and strongest on the ResourceOwner argument, which is genuinely a latent footgun for extensions. However, a common committer response to such "tighten the window" patches is: if the invariant matters, assert it; if it doesn't, don't churn the code. A reviewer might reasonably ask:

• Is there a demonstrable scenario (test, extension, or reproducer) where the current behavior causes a problem?
• Why a defensive clear in teardown if the batch-flush clear already guarantees the slot is empty? (Answer: only if every exit path from the batch flush clears; the defensive clear is cheap insurance.)
• Should the fix instead be an Assert that pk_slot is empty at teardown, documenting the invariant?

Linked Context

The reporter references a separate thread on AfterTriggerEndQuery debugging (CAAJ_b95p... message id), which is what led them to audit the fast-path code. That suggests the observation is a byproduct of unrelated investigation rather than a user-visible bug report, which further biases this toward a low-priority cleanup rather than a bug fix.

Assessment

This is a well-reasoned, narrowly scoped hygiene patch. The ResourceOwner argument is the one worth preserving in the commit message if the patch lands, because it articulates a real-but-latent extension-API hazard. Without follow-up responses in the thread (this is a single-message post), it is impossible to gauge committer reception, but the pattern — small, defensive, no measurable benefit, touches RI trigger hot path — is one that typically requires the author to either produce a failing test case or accept that the change may sit in the CF queue without champions.