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

Hi,

On 2026-09-29 16:31:39 +0000, Greg Burd wrote:
> From 057189b13e905377766f0e6c667c23849241708c Mon Sep 17 00:00:00 2001
> From: Greg Burd <greg(at)burd(dot)me>
> Date: Mon, 14 Sep 2026 12:58:22 -0400
> Subject: [PATCH v5 1/3] Restore tts_tid in ExecForceStoreHeapTuple()
>
> The TTS_IS_BUFFERTUPLE branch calls ExecClearTuple(), which resets
> tts_tid via tts_buffer_heap_clear(), then copies the tuple in without
> restoring tts_tid. Both tts_heap_store_tuple() and
> tts_buffer_heap_store_tuple() assign slot->tts_tid = tuple->t_self, so
> this reads as an omission rather than an intentional choice.
>
> It is user-visible because slot_getsysattr() answers
> SelfItemPointerAttributeNumber straight out of tts_tid. The reorder
> queue in nodeIndexscan.c reaches this path for any index AM that sets
> xs_recheckorderby, so an ORDER BY-op scan over such an AM projects
> (4294967295,0) as ctid. Feeding that sentinel to heap_lock_tuple()
> extends the relation, because InvalidBlockNumber equals P_NEW, leaving
> an uninitialized block that later breaks sequential scans.
>
> The bug dates to b8d71745eac, which added tts_tid and set it in both
> store callbacks while missing this branch. It only became observable
> at ff11e7f4b9a, which made tts_buffer_heap_clear() invalidate tts_tid;
> before that the slot retained a stale TID instead of the sentinel.
>
> The test uses thin diagonal triangles so that poly_ops' bounding-box
> distance is strictly below the true distance, which forces the requeue
> path, and materializes the ordered result before checking it so that
> the planner cannot push the checks' quals into the scan.
>
> Reported-by: Virender Singla
> Reported-by: Greg Burd

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.

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

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.

> From 7ed9738b2a8d2471673fbff88418fcfe411842f2 Mon Sep 17 00:00:00 2001
> From: Greg Burd <greg(at)burd(dot)me>
> Date: Mon, 14 Sep 2026 12:58:42 -0400
> Subject: [PATCH v5 2/3] Assert the reorder queue keeps a tuple's TID in the
> slot
>
> IndexNextWithReorder() re-stores a queued tuple with
> ExecForceStoreHeapTuple(), and slot_getsysattr() answers
> SelfItemPointerAttributeNumber out of tts_tid alone, so a slot that
> loses the TID silently projects a different ctid than the row it
> returned. Assert that the slot advertises the TID the tuple was
> fetched from.
>
> The invariant does not hold for slots in general, since HOT can
> legitimately make tts_tid and the stored tuple's t_self differ, so the
> check is confined to this path, where a divergence changes query
> results.
>
> The TID is captured before the store, which frees the tuple, and the
> comparison uses the NoCheck accessors so that the sentinel trips this
> assertion rather than the validity check inside ItemPointerEquals().

Maybe I'm slow, but who cares whether an earlier assert trips it? I think
flagging that the ctid is invalid, rather than unequal, seems perfectly fine?
Certainly doesn't seem worth to me to quadruple the size of the assertion for.

> From 1431683a3df66187dac74ededaf613f3d4ecaea6 Mon Sep 17 00:00:00 2001
> From: Greg Burd <greg(at)burd(dot)me>
> Date: Mon, 14 Sep 2026 13:01:10 -0400
> Subject: [PATCH v5 3/3] Reject an invalid TID before heap_lock_tuple() reads
> it
>
> An invalid TID reaching heap_lock_tuple() is handed to ReadBuffer() as
> InvalidBlockNumber, which is P_NEW, so the relation is extended by a
> block before the lock attempt fails. The uninitialized block is left
> behind and later breaks sequential scans with "invalid page in block".
> A caller that gets this far with such a TID has a bug, so fail cleanly
> instead.
>
> An earlier version of this check sat in table_tuple_lock(), but that was
> both too weak and in the wrong place. ItemPointerIsValid() only tests
> for a non-NULL pointer and a nonzero offset, so a TID like
> (InvalidBlockNumber, 1) passed it and still reached ReadBuffer(). And
> the hazard being guarded against is specific to heap's use of P_NEW,
> not a documented table AM invariant, so the generic layer is not the
> right place to enforce it. Checking the block number in
> heapam_tuple_lock() covers every caller of the heap AM's lock callback
> and leaves other AMs to state their own rules.

I don't think I buy the "specific to heap's use of P_NEW", given that

a) Stuff like ItemPointerGetBlockNumber() asserts out if P_NEW is used
b) P_NEW is a ReadBuffer() thing, not a heap thing

> The moved-partitions marker also encodes InvalidBlockNumber, with
> offset MovedPartitionsOffsetNumber, and is a legitimate value that the
> retry loop in heapam_tuple_lock() reports on its own terms, so it is
> allowed through.

I'm not sure I buy that? I don't think it'd be valid to call
heapam_tuple_lock() with something indicating a moved partition? That's
different than reaching that during the retry loop!

(not yet done thinking about this, but have to run into a meeting, and this
seemed enough)

Greetings,

Andres Freund