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
From ba97e9cb4b1c586ecbe6606f7c9f034d5dcef93b Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 14 Sep 2026 12:58:22 -0400
Subject: [PATCH v4 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
---
src/backend/executor/execTuples.c | 6 +++
src/test/regress/expected/gist.out | 61 ++++++++++++++++++++++++++++++
src/test/regress/sql/gist.sql | 47 +++++++++++++++++++++++
3 files changed, 114 insertions(+)
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index b8e8f52c64c..d38583a0507 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -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);
}
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index ac79f94aa80..192a5951e1a 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -463,3 +463,64 @@ create index gist_tbl_box_index on gist_tbl using gist (b);
insert into gist_tbl
select box(point(0.05*i, 0.05*i)) from generate_series(0,10) as i;
drop table gist_tbl;
+-- Test that tuples passing through nodeIndexscan.c's reorder queue keep their
+-- real ctid. poly_ops' distance is only a lower bound (the bounding box), so
+-- gist_poly_consistent sets recheck and the ORDER BY value is recomputed; thin
+-- diagonal triangles make the estimate strictly low, forcing the requeue path,
+-- which re-stores the tuple with ExecForceStoreHeapTuple(). Note the first
+-- tuple is returned without queueing, so any check must look past LIMIT 1.
+create table gist_knn_ctid (id int, p polygon);
+insert into gist_knn_ctid
+select i, format('((%s,0),(%s,9),(%s,0))', i * 10, i * 10 + 9, i * 10 + 9)::polygon
+from generate_series(1,20) i;
+create index gist_knn_ctid_idx on gist_knn_ctid using gist (p);
+vacuum analyze gist_knn_ctid;
+set enable_seqscan = off;
+-- the reorder queue is only reached through an ORDER BY-op index scan, so pin
+-- the plan that the checks below depend on
+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)
+
+-- Materialize the ordered result before checking it. Filtering the ordered
+-- subquery directly would let the planner push the qual into the scan, which
+-- would no longer exercise the same path.
+create temp table gist_knn_ctid_res as
+select ctid as c, id from gist_knn_ctid order by p <-> point(100,4) limit 5;
+-- no row may report the invalid-tid sentinel
+select count(*) as invalid_ctids from gist_knn_ctid_res
+where c = '(4294967295,0)'::tid;
+ invalid_ctids
+---------------
+ 0
+(1 row)
+
+-- every row must be findable by the ctid it reported
+select count(*) as ctid_matches
+from gist_knn_ctid_res r
+ join gist_knn_ctid t on t.ctid = r.c and t.id = r.id;
+ ctid_matches
+--------------
+ 5
+(1 row)
+
+-- and row locking must not be handed the invalid tid, which would ask
+-- ReadBuffer() for InvalidBlockNumber == P_NEW and extend the relation
+begin;
+select count(*) as locked
+from (select id from gist_knn_ctid order by p <-> point(100,4) limit 5
+ for update) s;
+ locked
+--------
+ 5
+(1 row)
+
+rollback;
+reset enable_seqscan;
+drop table gist_knn_ctid_res;
+drop table gist_knn_ctid;
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql
index 57dcc082450..91a73f1f076 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -236,3 +236,50 @@ create index gist_tbl_box_index on gist_tbl using gist (b);
insert into gist_tbl
select box(point(0.05*i, 0.05*i)) from generate_series(0,10) as i;
drop table gist_tbl;
+
+-- Test that tuples passing through nodeIndexscan.c's reorder queue keep their
+-- real ctid. poly_ops' distance is only a lower bound (the bounding box), so
+-- gist_poly_consistent sets recheck and the ORDER BY value is recomputed; thin
+-- diagonal triangles make the estimate strictly low, forcing the requeue path,
+-- which re-stores the tuple with ExecForceStoreHeapTuple(). Note the first
+-- tuple is returned without queueing, so any check must look past LIMIT 1.
+create table gist_knn_ctid (id int, p polygon);
+insert into gist_knn_ctid
+select i, format('((%s,0),(%s,9),(%s,0))', i * 10, i * 10 + 9, i * 10 + 9)::polygon
+from generate_series(1,20) i;
+create index gist_knn_ctid_idx on gist_knn_ctid using gist (p);
+vacuum analyze gist_knn_ctid;
+
+set enable_seqscan = off;
+
+-- the reorder queue is only reached through an ORDER BY-op index scan, so pin
+-- the plan that the checks below depend on
+explain (costs off)
+select ctid, id from gist_knn_ctid order by p <-> point(100,4) limit 5;
+
+-- Materialize the ordered result before checking it. Filtering the ordered
+-- subquery directly would let the planner push the qual into the scan, which
+-- would no longer exercise the same path.
+create temp table gist_knn_ctid_res as
+select ctid as c, id from gist_knn_ctid order by p <-> point(100,4) limit 5;
+
+-- no row may report the invalid-tid sentinel
+select count(*) as invalid_ctids from gist_knn_ctid_res
+where c = '(4294967295,0)'::tid;
+
+-- every row must be findable by the ctid it reported
+select count(*) as ctid_matches
+from gist_knn_ctid_res r
+ join gist_knn_ctid t on t.ctid = r.c and t.id = r.id;
+
+-- and row locking must not be handed the invalid tid, which would ask
+-- ReadBuffer() for InvalidBlockNumber == P_NEW and extend the relation
+begin;
+select count(*) as locked
+from (select id from gist_knn_ctid order by p <-> point(100,4) limit 5
+ for update) s;
+rollback;
+
+reset enable_seqscan;
+drop table gist_knn_ctid_res;
+drop table gist_knn_ctid;
--
2.50.1
From e60d12ffeda3ce747d0fb9e43f11cb29523efb38 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 14 Sep 2026 12:58:42 -0400
Subject: [PATCH v4 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().
Suggested-by: Andres Freund
---
src/backend/executor/nodeIndexscan.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 6566150cddc..410aefe3e0f 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -248,11 +248,32 @@ IndexNextWithReorder(IndexScanState *node)
node) <= 0)
{
HeapTuple tuple;
+ ItemPointerData tid PG_USED_FOR_ASSERTS_ONLY;
tuple = reorderqueue_pop(node);
+ /* Remember the TID; the store below frees the tuple. */
+ tid = tuple->t_self;
+
/* Pass 'true', as the tuple in the queue is a palloc'd copy */
ExecForceStoreHeapTuple(tuple, slot, true);
+
+ /*
+ * The tuple came from the heap through this scan, so the slot
+ * must advertise the TID it was fetched from. If the two
+ * diverge the scan projects a different ctid than the row it
+ * returned, which changes query results. This does not hold
+ * for slots in general, since HOT can legitimately make them
+ * differ, so assert it only here.
+ *
+ * Compare with the NoCheck accessors so that a slot left
+ * holding the invalid-TID sentinel trips this assertion rather
+ * than the validity one inside ItemPointerEquals().
+ */
+ Assert(ItemPointerGetBlockNumberNoCheck(&slot->tts_tid) ==
+ ItemPointerGetBlockNumberNoCheck(&tid) &&
+ ItemPointerGetOffsetNumberNoCheck(&slot->tts_tid) ==
+ ItemPointerGetOffsetNumberNoCheck(&tid));
return slot;
}
}
--
2.50.1
From 7a35a1e8816aff273809180143a3623358105d82 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 14 Sep 2026 13:01:10 -0400
Subject: [PATCH v4 3/3] Reject an invalid TID in table_tuple_lock()
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 an invalid TID has a bug, and nothing
good comes of letting it reach the AM, so check at the table AM
boundary where every AM is covered.
In an assert-enabled build the ItemPointerGetBlockNumber() inside
heap_lock_tuple() already trips on this, so the new check mainly buys a
clean error instead of relation extension in a production build.
Suggested-by: Andres Freund
---
src/include/access/tableam.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index ebec31a22e4..e362878387d 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -1652,6 +1652,19 @@ table_tuple_lock(Relation rel, ItemPointer tid, Snapshot snapshot,
LockWaitPolicy wait_policy, uint8 flags,
TM_FailureData *tmfd)
{
+ /*
+ * Reject an invalid TID here rather than letting it reach the AM. For
+ * heap that meant handing InvalidBlockNumber to ReadBuffer(), which is
+ * P_NEW and therefore extends the relation, leaving an uninitialized
+ * block behind that later breaks sequential scans. A caller that gets
+ * this far with an invalid TID has a bug, so fail cleanly instead.
+ */
+ if (unlikely(!ItemPointerIsValid(tid)))
+ elog(ERROR, "cannot lock tuple with invalid TID (%u,%u) in relation \"%s\"",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ RelationGetRelationName(rel));
+
return rel->rd_tableam->tuple_lock(rel, tid, snapshot, slot,
cid, mode, wait_policy,
flags, tmfd);
--
2.50.1
