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

Hi,

On 2026-10-01 10:16:05 -0400, Andres Freund wrote:
> On 2026-09-29 16:31:39 +0000, Greg Burd wrote:
> > @@ -1768,6 +1768,12 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
> > slot->tts_flags |= TTS_FLAG_SHOULDFREE;
> > MemoryContextSwitchTo(oldContext);
> >
> > + /*
> > + * ExecClearTuple() above reset tts_tid, so restore it from the tuple
> > + * we just stored, the same way the tts_*_store_tuple() callbacks do.
> > + */
> > + slot->tts_tid = tuple->t_self;
> > +
> > if (shouldFree)
> > pfree(tuple);
> > }
>
> So one thing that's weird is that this does not modify the subsequent else
> block:
>
> else
> {
> ExecClearTuple(slot);
> heap_deform_tuple(tuple, slot->tts_tupleDescriptor,
> slot->tts_values, slot->tts_isnull);
> ExecStoreVirtualTuple(slot);
>
> if (shouldFree)
> {
> ExecMaterializeSlot(slot);
> pfree(tuple);
> }
> }
>
> If we decide to maintain the tid in ExecForceStoreHeapTuple() - I suspect the
> right call - we really should actually do it consistently.

None of the copyslots maintains it properly either. Some just don't set it,
others will *sometimes* infer it from the source tuple
(e.g. tts_buffer_heap_copyslot, if the source slot is a buffer slot). Nor is
tts_tableOid consistently maintained as part of a copy.

That seems to currently not really be observable, because afaict we always
push down the projection of ctid, tableoid accesses down to the scan level,
where the slot is populated "originally" (if you assume nodeModifyTable.c is a
scan node, at least).

Which I guess is an argument to not consider that aspect a bug.

> The lock path itself can't be reached with a virtual slot (c.f. assertion in
> heapam_tuple_lock()), but it might be possible to have a virtual slot in the
> path towards it. But even if it can't be today, it'll likely be soon.

AI did find a path, see attached tid2.sql, note the wrong tid for
INSERT INTO parent_rows VALUES (2) RETURNING a, ctid;

Seems we should just move
slot->tts_tid = tuple->t_self;
to the end of ExecForceStoreHeapTuple()?

After looking at tid2.sql I played around trying to see whether it'd make it
possible to get an invalid tid for an update and was rather horrified to find
out that cross-partition updates involving ctids just silently cause data to
be inconsistent with the partition constraints :(.

Greetings,

Andres Freund