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

On Thursday, October 1st, 2026 at 12:58 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,

Hello Andres, thanks for looking into this.

> We normally include email addresses as well. Also would be good to add
> Discussion links for the (at least?) two threads. Adding Reviewed-By also
> helps eventual committers.

Yep, done, addresses on everything, both Discussion links, and Reviewed-by for
you, Dilip, Nik and Virender in v7 attached. What's the convention when someone
both reports an issue and co-authors the patch? I added myself and Virender
as reporters, but maybe we're also co-authors too? Still learning this... :)

I'm dropping the attempts at adding asserts in other paths mainly because I'm
not sure of the value but also because while testing I ran into b8b94ea129f:

For amcanreorderby scans the nodeIndexscan.c's reorder queue holds
heap tuples, but the underlying table likely does not.

For such an AM the scan slot isn't a buffer slot, so reorderqueue_push() goes
ExecCopySlotHeapTuple() -> tts_virtual_copy_heap_tuple() -> heap_form_tuple(),
which does ItemPointerSetInvalid(&tuple->t_self). The queued tuple therefore
has no TID to begin with. 0001 stores that invalid TID and the assertion then
fires on a slot that is behaving correctly. heap_copytuple() preserves t_self,
which is why heap never hits this.

> 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.

Agreed.

> > 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()?

I took a copy of the TID at function start and use that later to avoid a case
where the tuple might have bee free'd.

> 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 :(.

Yikes, good find that sounds worse than anything in this thread. The
postgres_fdw side looks like a separate patch, and I don't think it
should hold up the backpatchable fix. I'll take a look at it
separately if nobody else is already on it and start a new thread.

> Greetings,
>
> Andres Freund

v7/0001 the fix, now maintaining the TID for all slot types, plus the
regression test. This is the piece that wants backpatching.

On backpatching 0001: applies as-is to 18, 17, 16 and 15. On 14 the C
hunk applies with an offset but the test hunks don't, gist.sql has grown
since. The test block is self-contained, so appending it to the end of
14's gist.sql and gist.out is enough IMO.

best.

-greg