[PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple

Just a quick ping on this patch.
It appears the `tts_tid` assignment was inadvertently omitted from the
BufferHeapTupleTableSlot path in ExecForceStoreHeapTuple() when the
function was first introduced during the PG12 TupleTableSlot
refactoring [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da597edf1b.
This was subsequently exposed when the GiST index scan reorder queue
was updated to route popped tuples through this same function [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b8b94ea129f. The
relevant code hasn't changed since those commits.

Thanks,
Virender
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da597edf1b
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b8b94ea129f

On Sep 14, 2026, at 8:10 AM, Virender Singla <virender.cse@gmail.com> wrote:

Just a quick ping on this patch.
It appears the `tts_tid` assignment was inadvertently omitted from the
BufferHeapTupleTableSlot path in ExecForceStoreHeapTuple() when the
function was first introduced during the PG12 TupleTableSlot
refactoring [1].

I ran into this myself and posted a patch [1]https://pg.ddx.io/m/pgsql-hackers/0498c10f-839b-4f68-9994-c29b454e55a4@app.fastmail.com/ as well. I had not noticed your
patch. Looks like both patches are essentially identical with different tests
we could combine those into one and then possibly get the attention of a
committer.

This was subsequently exposed when the GiST index scan reorder queue
was updated to route popped tuples through this same function [2]. The
relevant code hasn't changed since those commits.

Thanks,
Virender
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da597edf1b
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b8b94ea129f

I agree, it's a bug/oversight that has lingered since v12 and should be fixed.

best.

-greg

[1]: https://pg.ddx.io/m/pgsql-hackers/0498c10f-839b-4f68-9994-c29b454e55a4@app.fastmail.com/

On Mon, Sep 14, 2026 at 5:47 PM Burd, Greg <greg@burd.me> wrote:

On Sep 14, 2026, at 8:10 AM, Virender Singla <virender.cse@gmail.com> wrote:

Just a quick ping on this patch.
It appears the `tts_tid` assignment was inadvertently omitted from the
BufferHeapTupleTableSlot path in ExecForceStoreHeapTuple() when the
function was first introduced during the PG12 TupleTableSlot
refactoring [1].

I ran into this myself and posted a patch [1] as well. I had not noticed your
patch. Looks like both patches are essentially identical with different tests
we could combine those into one and then possibly get the attention of a
committer.

This was subsequently exposed when the GiST index scan reorder queue
was updated to route popped tuples through this same function [2]. The
relevant code hasn't changed since those commits.

Thanks,
Virender
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da597edf1b
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b8b94ea129f

I agree, it's a bug/oversight that has lingered since v12 and should be fixed.

+1 this seems like and issue, we can compare this with
ExecStoreHeapTuple() which restore back the ctid so I think here it
seems like and oversight and attached patch seems to be doing right
thing.

--
Regards,
Dilip Kumar
Google

On Mon, Sep 14, 2026, at 8:31 AM, Dilip Kumar wrote:

On Mon, Sep 14, 2026 at 5:47 PM Burd, Greg <greg@burd.me> wrote:

On Sep 14, 2026, at 8:10 AM, Virender Singla <virender.cse@gmail.com> wrote:

Just a quick ping on this patch.
It appears the `tts_tid` assignment was inadvertently omitted from the
BufferHeapTupleTableSlot path in ExecForceStoreHeapTuple() when the
function was first introduced during the PG12 TupleTableSlot
refactoring [1].

I ran into this myself and posted a patch [1] as well. I had not noticed your
patch. Looks like both patches are essentially identical with different tests
we could combine those into one and then possibly get the attention of a
committer.

This was subsequently exposed when the GiST index scan reorder queue
was updated to route popped tuples through this same function [2]. The
relevant code hasn't changed since those commits.

Thanks,
Virender
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=4da597edf1b
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b8b94ea129f

I agree, it's a bug/oversight that has lingered since v12 and should be fixed.

+1 this seems like and issue, we can compare this with
ExecStoreHeapTuple() which restore back the ctid so I think here it
seems like and oversight and attached patch seems to be doing right
thing.

Hey Dilip, thanks for chiming in.

--
Regards,
Dilip Kumar
Google

Virender,

I've re-worked the tests to include your two checks, and found something
about them worth flagging: at LIMIT 1 neither one fails on unpatched code.

The first tuple out of IndexNextWithReorder() is the one whose advertised
distance compared equal to the recomputed one, so was_exact is true and it
never enters the reorder queue. It keeps its real ctid. Only the tuples
behind it get requeued through ExecForceStoreHeapTuple(). I built your
test against a tree with the execTuples.c hunk reverted and valid_tid
comes back 't', and the FOR UPDATE succeeds. Raise the same table to
LIMIT 5 and 4 of 5 rows are (4294967295,0) and the FOR UPDATE dies. Your
checks are right, it's just the LIMIT.

So v2 keeps both of your assertions at LIMIT 5, on the polygon fixture
from my patch, plus an EXPLAIN to pin the plan (worth having, since your
block landed after the reset enable_seqscan at the end of gist.sql). A/B
with only the execTuples.c hunk reverted:

unpatched patched
sentinel ctids at LIMIT 5 4 0
ctid self-join, expect 5 1 5
ORDER BY ... LIMIT 5 FOR UPDATE assert 5

I dropped the separate circle_ops table. Unpatched it fails at LIMIT 5
just like the polygon one (19 of 20 sentinels), same reorderqueue_pop
path, so it wasn't buying coverage. The polygon case is only a stronger
trigger in degree.

The FOR UPDATE case is important. I reproduced the assert
(ItemPointerIsValid in itemptr.h). The non-assert consequence, P_NEW
extending the relation and leaving a block that later breaks seqscans,
I've taken from your mail rather than tested and credited as such.
It's the strongest argument for back-patching, since it turns a
wrong-answer bug into on-disk damage.

One refinement on provenance. 4da597edf1b did create the function without
the assignment, but tts_tid didn't exist yet. The field arrives in
b8d71745eac, which sets it in both tts_heap_store_tuple and
tts_buffer_heap_store_tuple and misses this branch, and it only becomes
observable at ff11e7f4b9a, which made tts_buffer_heap_clear invalidate
tts_tid. Before that ExecClearTuple left it alone, so the slot kept a
stale value instead of the sentinel. Then b8b94ea129f made it reachable,
as you say, by dropping iss_ReorderQueueSlot, which was TTSOpsHeapTuple
and took the correct branch. All in the v12 cycle, so your conclusion
holds, just a different commit as the origin.

On that basis I've also switched to your Backpatch-through: 14 rather
than the 13 I'd written, since 13 is out of support.

On author attribution, you first since your post predates mine. Dilip,
thanks for the review.

v3 attached,

-greg

Hi,

On 2026-09-01 11:48:43 +0530, Virender Singla wrote:

-- 2. Fails with "attempted to lock invisible tuple" and extends table on disk:

I was a bit shocked about the extends-table-on-disk issue at first, thinking
it'd lead to trying to extend the relation to an absurd size. But it's "just"
the invalid block number being interpreted as a request to extend the relation
by one block (because our historical relation extension path was to call
ReadBuffer() with P_NEW, which is InvalidBlockNumber). So this isn't *too*
bad.

I think we, separately from the fix to main tts_tid, should also add an error
path against trying to lock an invalid tid. This should have never gotten
anywhere close to a ReadBuffer() IMO.

So v2 keeps both of your assertions at LIMIT 5, on the polygon fixture
from my patch, plus an EXPLAIN to pin the plan (worth having, since your
block landed after the reset enable_seqscan at the end of gist.sql).

Yep, I think verifying plans in stuff like this is a good practice.

One refinement on provenance. 4da597edf1b did create the function without
the assignment, but tts_tid didn't exist yet. The field arrives in
b8d71745eac, which sets it in both tts_heap_store_tuple and
tts_buffer_heap_store_tuple and misses this branch, and it only becomes
observable at ff11e7f4b9a, which made tts_buffer_heap_clear invalidate
tts_tid. Before that ExecClearTuple left it alone, so the slot kept a
stale value instead of the sentinel.

Which is *way* worse.

+create table gist_knn_ctid (id int, p polygon);
+insert into gist_knn_ctid
+select i, ('((' || i*10 || ',0),(' || (i*10+9) || ',9),('
+               || (i*10+9) || ',0))')::polygon

Ick, that's hard to read. Maybe a format() or such would make it easier?

+from generate_series(1,20) i;
+create index gist_knn_ctid_idx on gist_knn_ctid using gist (p);
+vacuum analyze gist_knn_ctid;
+explain (costs off)
+select ctid, id from gist_knn_ctid order by p <-> point(100,4) limit 5;
+                        QUERY PLAN                         
+-----------------------------------------------------------
+ Limit
+   ->  Index Scan using gist_knn_ctid_idx on gist_knn_ctid
+         Order By: (p <-> '(100,4)'::point)
+(3 rows)
+

+-- no row may report the invalid-tid sentinel
+select count(*) as invalid_ctids
+from (select ctid from gist_knn_ctid order by p <-> point(100,4) limit 5) s
+where ctid = '(4294967295,0)'::tid;

Is that actually reliable? In theory the filter could get pushed down
(although unlikely due to the limit and the complexity safely pushing down
below that would require), and you don't have an explain plan guarding it.

I'd probably put the query results in a temp table and then query that table
in the verifications.

+ invalid_ctids 
+---------------
+             0
+(1 row)
+
+-- every row must be findable by the ctid it reported
+select count(*) as ctid_matches
+from (select ctid, id from gist_knn_ctid order by p <-> point(100,4) limit 5) s
+     join gist_knn_ctid t on t.ctid = s.ctid and t.id = s.id;
+ ctid_matches 
+--------------
+            5
+(1 row)

I'd make this a NOT EXISTS() or such, a 0-rows-not-found is easier to verify.

Greetings,

Andres Freund

Hello Andres,

Thanks for the review. Attached is v4.

0001 is the fix, plus the regression test. One line: restore tts_tid from
the tuple in the TTS_IS_BUFFERTUPLE branch of ExecForceStoreHeapTuple(),
which is what both tts_heap_store_tuple() and tts_buffer_heap_store_tuple()
already do. This is the piece that wants backpatching.

0002 is the assertion you suggested, in IndexNextWithReorder() only.

0003 is the invalid-TID error path you asked for, at the table AM boundary.

More detail on each, and the review points, below.

I think we, separately from the fix to main tts_tid, should also add an error
path against trying to lock an invalid tid. This should have never gotten
anywhere close to a ReadBuffer() IMO.

That is 0003. I put the check in table_tuple_lock() rather than
heap_lock_tuple(), because heap_lock_tuple() is only reachable through the
AM callback in heapam_handler.c, so one check at the boundary covers every
AM and rejects the TID before any AM code runs.

I tested with 0001 reverted and only 0003 in place, the FOR UPDATE case gives

ERROR: cannot lock tuple with invalid TID (4294967295,0) in relation "tri"

and pg_relation_size() reads 385024 bytes, 47 blocks, both before and after,
so no extension, and the backend stays up. Remove 0003 from the same
assert-enabled build and it crashes instead, tripping
ItemPointerIsValid() inside ItemPointerGetBlockNumber() called from
heap_lock_tuple(). I did not rebuild without assertions, so the production
extension behaviour is still Virender's report rather than mine; the commit
message says as much.

I wonder if we ought to have an assertion for the two tids being the same
that, perhaps only on master?
I don't think we can do that in general, there are legitimate cases of those
differing due to HOT IIRC. But in the reorder case I don't think that
difference exists [...] so I think we should just assert it there.

0002 is what I think you're asking for, in IndexNextWithReorder() only.

My first attempt asserted the general invariant in slot_getsysattr(), for any
heap or buffer-heap slot holding a tuple, and it passed the whole suite under
cassert with nothing firing. So whatever divergence exists is not exercised
anywhere in the tests. That does not make the general version safe, and I have
not tried to argue for it here, but if someone revisits this later that is the
starting point.

One trap worth recording for whoever writes this next. The obvious form of
the assertion is wrong:

ExecForceStoreHeapTuple(tuple, slot, true);
Assert(ItemPointerEquals(&slot->tts_tid, &tuple->t_self)); /* wrong */

shouldFree is true here, so the store pfrees the tuple and t_self is read
from freed memory. It shows up as a TID of (0,32639), which is 0x7F7F, the
clobber pattern. 0002 copies the TID out before the store. It also compares
with the NoCheck accessors, because ItemPointerEquals() asserts validity
internally and would otherwise trip on that rather than on the thing we
actually want to catch.

Ick, that's hard to read. Maybe a format() or such would make it easier?

Better?

select i, format('((%s,0),(%s,9),(%s,0))', i * 10, i * 10 + 9, i * 10 + 9)::polygon

I'd probably put the query results in a temp table and then query that table
in the verifications.

Done. The ordered result goes into a temp table first and the checks run
against that, so there is no qual left for the planner to push into the
ordered scan. The EXPLAIN stays, to pin the plan that produces the temp
table. I also added an explicit set/reset of enable_seqscan, since the block
lands after gist.sql's final reset and would otherwise not be guaranteed an
index scan.

I'd make this a NOT EXISTS() or such, a 0-rows-not-found is easier to verify.

Done, so both checks now read zero when correct:

select count(*) as invalid_ctids from gist_knn_ctid_res
where c = '(4294967295,0)'::tid;

select count(*) as rows_not_found
from gist_knn_ctid_res r
where not exists (select 1 from gist_knn_ctid t
where t.ctid = r.c and t.id = r.id);

The test does gate the fix, which I checked at 0001 alone rather than with
the assertion masking it: reverting the one line takes invalid_ctids from 0
to 4 and rows_not_found from 0 to 4, and the FOR UPDATE aborts. Four of five
rather than five is the was_exact fast path returning the first tuple
without queueing, which is why the comment warns against checking at
LIMIT 1.

Which is *way* worse.

Agreed, and that is in 0001's commit message now: the omission dates to
b8d71745eac, which added tts_tid and set it in both store callbacks while
missing this branch, and it only became observable at ff11e7f4b9a, which
made tts_buffer_heap_clear() invalidate tts_tid. Before that the slot kept a
stale TID, which is worse than a recognisable sentinel and would have been
much harder to spot.

One loose end, for Michael's earlier question about tts_buffer_heap_copyslot()
having the same clear-then-copy shape. It does, and its copy branch also
never restores tts_tid, while the branch below it goes through
tts_buffer_heap_store_tuple() and does. I wrote the fix, then instrumented
that branch to see whether it is reachable with a source tuple that has a
valid t_self, and got zero hits across the regress and recovery suites. So I
have left it alone rather than ship a change I cannot demonstrate. If anyone
can construct a case that reaches it, it should be fixed the same way.

The whole series builds clean with no warnings, each commit builds on its
own, and the full suite is green under cassert here on macOS/arm64: 389
passed, 0 failed, 23 skipped, where the skips want ssl/ldap/xid_wraparound
setup I do not have.

best.

-greg

Hi Greg,

Thank you for driving this forward and taking care of the refinements!
I applied the v4 patchset locally and verified that it correctly fixes
the original issue and passes all tests.

Thanks,
Virender

On Tue, Sep 15, 2026, at 5:42 AM, Virender Singla wrote:

Hi Greg,

Thank you for driving this forward and taking care of the refinements!

Hey Virender, I'm happy to help!

I applied the v4 patchset locally and verified that it correctly fixes
the original issue and passes all tests.

Thanks for the review.

Thanks,
Virender

best.

-greg