Re: [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple

On Sat, 31 Oct 2026 10:50:46 +0200, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 30/10/2026 18:11, Nathan Bossart wrote:
> > --- a/src/backend/executor/execTuples.c
> > +++ b/src/backend/executor/execTuples.c
> > @@ -1752,6 +1752,8 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
> > TupleTableSlot *slot,
> > bool shouldFree)
> > {
> > + ItemPointerData tid = tuple->t_self;
> > +
> > if (TTS_IS_HEAPTUPLE(slot))
> > {
> > ExecStoreHeapTuple(tuple, slot, shouldFree);
> > @@ -1784,6 +1786,13 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
> > pfree(tuple);
> > }
> > }
> > +
> > + /*
> > + * ExecClearTuple() invalidated the slot's TID, so we need to set it again
> > + * here. Note that the tuple might have already been freed, so we use the
> > + * copy of its TID that we saved above.
> > + */
> > + slot->tts_tid = tid;
> > }
> >
> > /*
>

Hey Heikki, thanks for taking a look at this. I appreciate it.

> The comment sounds wrong to me. This isn't restoring the old TID that
> was in the slot previously, it's setting the new TID from 'tuple'. Or am
> I missing something?

No, you're right. ExecClearTuple() resets the slot's previous TID, and
what goes in afterwards is the incoming tuple's t_self, which has no
relation to the old value. "set it again" reads as restoring it.

>
> For same reason, the local variable seems pointless; you could just do:
>
> slot->tts_tid = tuple->t_self;

That's the form I tried first, at Andres's suggestion, and it's why the
local is there. The tuple is gone by the end of the function. Both the
buffer and the virtual branches finish with

if (shouldFree)
pfree(tuple);

and the only two callers that pass shouldFree = true are the ones this
patch is about:

nodeIndexscan.c:257 ExecForceStoreHeapTuple(tuple, slot, true);
postgres_fdw.c:4660 ExecForceStoreHeapTuple(newtup, slot, true);

the reorder queue, and Andres's tid2.sql routed insert. So reading
tuple->t_self after the if/else reads freed memory.

I re-ran it this morning on Debian 13, master at 77705d1e5a, with the
GiST reorder-queue case from the thread (20 thin triangles, ORDER BY p
<-> point(100,4) LIMIT 5). With your form, an assert-enabled build gives

ctid | id
--------------------+----
(0,9) | 9
(0,32639) | 10
(0,32639) | 11
(1437663232,32639) | 8
(0,32639) | 12

32639 is 0x7F7F, the CLOBBER_FREED_MEMORY fill, and the row with the
huge block number has picked up a pointer from whatever reused the
chunk. Without assertions it doesn't fail cleanly either: four rows come
out right, and one gets a different random block each time,
(1432158208,8) on one run and (1440808960,8) on the next. With the local
copy (v10) all five rows are (0,9) (0,10) (0,11) (0,8) (0,12), on both
builds and over three runs each.

>
> The comment in Greg's earlier version (v9) said:
>
> > + /*
> > + * Store the TID from the local copy taken on entry since the stores above
> > + * may have freed the tuple.
> > + */
> > + slot->tts_tid = tid;
>
> But I don't see how the store might've free'd the tuple. If it did, we'd
> have trouble anyway.

That comment was badly worded, and I think it's what sent you down this
path. The stores don't free it; the explicit pfree() after each store
does. And we don't have trouble anyway, because each branch copies the
data out before freeing it: heap_copytuple() in the buffer branch and
ExecMaterializeSlot() in the virtual one. The slot is fine. Only the
original tuple is gone, and t_self is the one thing we still read from
it.

So I'd keep the local copy and fix the comment so it gives the actual
reason, something like:

/*
* Set the slot's TID from the tuple. ExecClearTuple() reset it, and
* only the heap-slot branch above stores a new one. Use the copy taken
* on entry, because the branches above free the tuple when
* shouldFree is true.
*/
slot->tts_tid = tid;

> - Heikki

Nathan, please adjust the wording however you like when you commit.

best.

-greg