Fix resource leak in FindConflictTuple() retry path

On Thu, Sep 3, 2026 at 9:22 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

Hi,

As part of the AI-assisted review of the update_deleted conflict
detection work [1], a small resource leak was identified in
FindConflictTuple(), introduced by commit 9758174e2e5.

When should_refetch_tuple() returns true i.e. when the conflicting
tuple was modified between ExecCheckIndexConstraints() and
table_tuple_lock(), the function retries from its retry label. On each
pass it creates a new slot and assigns it to *conflictslot,
overwriting the previous slot without releasing it. So every retry
abandons one slot.

The abandoned slot also holds a buffer pin. Since FindConflictTuple()
does not pass TUPLE_LOCK_FLAG_FIND_LAST_VERSION, heapam_tuple_lock()
falls through to ExecStorePinnedBufferHeapTuple(), which transfers the
pin to the slot even for TM_Updated. The heap_lock_tuple() failure
path releases the content lock but not the pin. Since the slot has a
NULL reglist, it is not registered in estate->es_tupleTable and is
therefore not cleaned up by ExecResetTupleTable().

This is mostly harmless in practice: the slot is freed with its memory
context, and buffer pins are released at transaction end. The usual
conflict path also aborts the transaction with ERROR, releasing
everything.

Still, a pinned buffer cannot be evicted, and repeated retries on a
contended unique key can accumulate pins for the lifetime of the
transaction, so this seems worth fixing.

The attached patch-001 creates the slot once before the retry loop and
reuses it. Re-storing a tuple in the same slot releases its previous
buffer pin, so no slot is abandoned.

Reproducing the issue:
The window is very narrow and cannot be triggered directly from SQL,
as the concurrent UPDATE must occur between
ExecCheckIndexConstraints() and table_tuple_lock() inside
FindConflictTuple().

I created a small, hacky TAP test (patch-002) with the help of Claude,
which uses an injection point and elog() to reproduce the issue and
verify slot reuse. This test is only for demonstrating the problem and
is not intended for commit.

Since this is an oversight in commit 9758174e2e5, it should be
backpatched to PG18.

Feedback on the fix and approach is welcome.

[1] /messages/by-id/TY4PR01MB177182F547A62FC2666EC04EC94B72@TY4PR01MB17718.jpnprd01.prod.outlook.com

I agree with the idea of patch. But the patch can be improved. Before
this patch, slot creation happened after the conflict was found. Now
it happens unconditionally. So every call to FindConflictTuple() now
allocates a slot and immediately tears it down. So most of the cases
which are ocnflict-free now will do slot-allocation. I feel this can
be optimized.

Suggestion:

retry:
if (ExecCheckIndexConstraints(...))
{
if (*conflictslot)
ExecDropSingleTupleTableSlot(*conflictslot);
*conflictslot = NULL;
return false;
}

if (*conflictslot == NULL)
*conflictslot = table_slot_create(rel, NULL);

thanks
Shveta

On Thu, Sep 3, 2026 at 10:54 AM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 3, 2026 at 9:22 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

I agree with the idea of patch. But the patch can be improved. Before
this patch, slot creation happened after the conflict was found. Now
it happens unconditionally. So every call to FindConflictTuple() now
allocates a slot and immediately tears it down. So most of the cases
which are ocnflict-free now will do slot-allocation. I feel this can
be optimized.

Good point, agree.

Suggestion:

retry:
if (ExecCheckIndexConstraints(...))
{
if (*conflictslot)
ExecDropSingleTupleTableSlot(*conflictslot);
*conflictslot = NULL;
return false;
}

if (*conflictslot == NULL)
*conflictslot = table_slot_create(rel, NULL);

Adopted in v2, attached.

--
Thanks,
Nisha

Dear Nisha,

Not sure, should we release a buffer pin if should_refetch_tuple() returns true?
I referred heapam_tuple_lock()/heap_lock_tuple(), they pin a buffer via ReadBuffer()
and transfers to a slot via ExecStorePinnedBufferHeapTuple().

IIUC, ExecClearTuple() can release corresponding resources for the slot, so below
fix is enough.

```
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -268,7 +268,10 @@ retry:
                PopActiveSnapshot();

                if (should_refetch_tuple(res, &tmfd))
+               {
+                       ExecClearTuple(*conflictslot);
                        goto retry;
+               }
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Sep 3, 2026 at 1:03 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Nisha,

Not sure, should we release a buffer pin if should_refetch_tuple() returns true?
I referred heapam_tuple_lock()/heap_lock_tuple(), they pin a buffer via ReadBuffer()
and transfers to a slot via ExecStorePinnedBufferHeapTuple().

IIUC, ExecClearTuple() can release corresponding resources for the slot, so below
fix is enough.

IMO, ExecClearTuple() can resolve the problem of pinned buffers but it
still leaves the other trivial problem behind that we keep allocating
slots while one is enough, which when resued will release buffer pin
in ExecStorePinnedBufferHeapTuple() -> tts_buffer_heap_store_tuple()
implicitly.
Therefore, this alone might not be a better fix. But if we want to
club it with the fix already provided by Nisha, that will help us
releasing the pin a little earlier without relying on next
table_tuple_lock() on same slot to do that implictly. That said, I
don't see ExecClearTuple() called explicitly in the two similar
functions in this file, RelationFindReplTupleByIndex() and
RelationFindReplTupleSeq(); both of which reuse a single
caller-provided slot across retries and rely purely on the implicit
release-on-restore behavior.

On Thu, Sep 3, 2026 at 2:36 PM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 3, 2026 at 1:03 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:

Dear Nisha,

Not sure, should we release a buffer pin if should_refetch_tuple() returns true?
I referred heapam_tuple_lock()/heap_lock_tuple(), they pin a buffer via ReadBuffer()
and transfers to a slot via ExecStorePinnedBufferHeapTuple().

IIUC, ExecClearTuple() can release corresponding resources for the slot, so below
fix is enough.

IMO, ExecClearTuple() can resolve the problem of pinned buffers but it
still leaves the other trivial problem behind that we keep allocating
slots while one is enough, which when resued will release buffer pin
in ExecStorePinnedBufferHeapTuple() -> tts_buffer_heap_store_tuple()
implicitly.

+1

Therefore, this alone might not be a better fix. But if we want to
club it with the fix already provided by Nisha, that will help us
releasing the pin a little earlier without relying on next
table_tuple_lock() on same slot to do that implictly. That said, I
don't see ExecClearTuple() called explicitly in the two similar
functions in this file, RelationFindReplTupleByIndex() and
RelationFindReplTupleSeq(); both of which reuse a single
caller-provided slot across retries and rely purely on the implicit
release-on-restore behavior.

I also agree not to make FindConflictTuple() the only one clearing explicitly.

--
Thanks,
Nisha

Dear Nisha, Shveta,

IMO, ExecClearTuple() can resolve the problem of pinned buffers but it
still leaves the other trivial problem behind that we keep allocating
slots while one is enough, which when resued will release buffer pin
in ExecStorePinnedBufferHeapTuple() -> tts_buffer_heap_store_tuple()
implicitly.

+1

Sorry for missing words, my intention was to additionally modify atop a Nisha's fix.
Had no objections for proposed patch.

Therefore, this alone might not be a better fix. But if we want to
club it with the fix already provided by Nisha, that will help us
releasing the pin a little earlier without relying on next
table_tuple_lock() on same slot to do that implictly. That said, I
don't see ExecClearTuple() called explicitly in the two similar
functions in this file, RelationFindReplTupleByIndex() and
RelationFindReplTupleSeq(); both of which reuse a single
caller-provided slot across retries and rely purely on the implicit
release-on-restore behavior.

Okay, I have not seen that. Then either fixing or retain all is OK for me.

Best regards,
Hayato Kuroda
FUJITSU LIMITED