On Monday, September 21st, 2026 at 12:24 AM, Nikolay Samokhvalov
<[email protected]> wrote:
Hi Nik,
Thanks for reviewing, both of your points are correct. Thanks, and sorry for
the slow reply.
> My AI harness noticed that v4-0001 still has the old ctid_matches join
> returning 5, in both gist.sql and gist.out. It looks like the email and
> attachment got out of sync.
Right, and the email was the thing that was wrong. I did the format() and
temp-table changes, wrote up all three, and never actually made the
NOT EXISTS change before generating the patch. Fixed in v5-0001:
select count(*) as ctid_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);
So both checks now read zero when correct, which is what I claimed last
time.
> This catches the reported (InvalidBlockNumber, 0), but
> ItemPointerIsValid() only checks for a non-NULL pointer and ip_posid != 0.
> For example, (InvalidBlockNumber, 1) still reaches the AM.
Yes. ItemPointerIsValid() is just
return pointer && pointer->ip_posid != 0;
So my check tested the offset and called it a block-number guard. Anything
with InvalidBlockNumber and a nonzero offset went straight through to
ReadBuffer() as P_NEW, which is exactly the case I said I was preventing.
> If the intended protection is specifically against passing P_NEW to
> heap's ReadBuffer(), should this check be heap-side? A stronger generic
> check would need a clearly stated table-AM invariant; the moved-partitions
> marker is also an InvalidBlockNumber encoding with a nonzero offset.
Agreed on both halves, and I've moved it. v5-0003 checks the block number
in heapam_tuple_lock() instead of table_tuple_lock(). That covers every
caller of the heap AM's lock callback, since heap_lock_tuple() is only
reachable through it, and it leaves other AMs to state their own rules
rather than my inventing an invariant for them.
Your moved-partitions point turned out to matter in a second way I hadn't
anticipated. MovedPartitionsBlockNumber is InvalidBlockNumber with offset
0xfffd, so it has to be allowed through to the existing report in
heapam_tuple_lock()'s retry loop. But the obvious spelling,
if (ItemPointerGetBlockNumberNoCheck(tid) == InvalidBlockNumber &&
!ItemPointerIndicatesMovedPartitions(tid))
crashes before it can reject anything: ItemPointerIndicatesMovedPartitions()
reads the offset through the checked accessor, which asserts on offset 0,
which is the case we're here to catch. The offset has to be tested first.
I only found that because the assert fired; reading the code I had
convinced myself it was fine.
v5 attached, rebased on master (6a93535798a).
0001 the fix, plus the regression test. This is the piece that wants
backpatching.
0002 the reorder-case assertion Andres asked for. master only.
0003 the invalid-TID rejection, now heap-side. master only.
On backpatching 0001: it applies as-is to 18, 17, 16 and 15. On 14 the C
hunk applies (with an offset) but the test hunks do not, because gist.sql
has grown since, the block ahead of the insertion point in master does not
exist on 14. The test block itself is self-contained, its own table, its
own set/reset of enable_seqscan, its own cleanup, no dependency on
gist_tbl, so appending it to the end of 14's gist.sql and gist.out is all
that is needed.
I built that assembled 14 tree with --enable-cassert to check I wasn't
just asserting this. The gist test passes with the fix, and with only the
execTuples.c hunk reverted it fails the way it should:
invalid_ctids 0 -> 4
ctid_not_found 0 -> 4
FOR UPDATE 5 rows -> assert
So the test gates the fix on 14 as well, it just needs placing by hand.
Happy to send a separate 14-specific 0001 if that's easier for whoever
commits it.
Verified on an assert-enabled build:
- 0003 alone, with 0001 and 0002 reverted: the FOR UPDATE case gives
ERROR: cannot lock tuple with invalid TID (4294967295,0) in relation
"tri", pg_relation_size() is 8192 before and after, and the backend
stays up. Without 0003 the same build asserts in
ItemPointerGetOffsetNumber() under heapam_tuple_lock().
- (4294967295,65533), the moved-partitions encoding, still reaches its
own path rather than the new error.
- All three applied: regression suite green, 239 tests.
I have still not built without assertions, so the production
relation-extension behavior remains Virender's report rather than
something I've reproduced. The commit message says so.
One loose end: there's no commitfest entry for this thread. I'll create
one so it doesn't get lost, unless Virender would rather do it as the
original reporter.
best.
-greg
From 1431683a3df66187dac74ededaf613f3d4ecaea6 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
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.
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. Note the offset has to be tested before calling
ItemPointerIndicatesMovedPartitions(), which reads it through the
checked accessor and would assert on an offset of zero.
In an assert-enabled build 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 <[email protected]>
Reported-by: Nikolay Samokhvalov <[email protected]>
Discussion: https://postgr.es/m/CAM6Zo8wZOLnCWRO_tuuXVX9J4N4JN6GsEnk8WJtT0%3D_0zy-1dw%40mail.gmail.com
---
src/backend/access/heap/heapam_handler.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 6adb760b54f..7b3467ef65b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -278,6 +278,27 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
Assert(TTS_IS_BUFFERTUPLE(slot));
+ /*
+ * Reject a TID that does not name a heap block before it reaches
+ * ReadBuffer() below, which would read InvalidBlockNumber as P_NEW and
+ * extend the relation, leaving an uninitialized block behind that later
+ * breaks sequential scans with "invalid page in block". A caller that
+ * gets here with such a TID has a bug, so fail cleanly instead.
+ *
+ * The moved-partitions marker also encodes InvalidBlockNumber, but it is
+ * a legitimate value that the retry loop below reports on its own terms,
+ * so let it through. Test the offset first, since
+ * ItemPointerIndicatesMovedPartitions() reads it through the checked
+ * accessor, which would assert on an offset of 0.
+ */
+ if (unlikely(ItemPointerGetBlockNumberNoCheck(tid) == InvalidBlockNumber &&
+ (ItemPointerGetOffsetNumberNoCheck(tid) == InvalidOffsetNumber ||
+ !ItemPointerIndicatesMovedPartitions(tid))))
+ elog(ERROR, "cannot lock tuple with invalid TID (%u,%u) in relation \"%s\"",
+ ItemPointerGetBlockNumberNoCheck(tid),
+ ItemPointerGetOffsetNumberNoCheck(tid),
+ RelationGetRelationName(relation));
+
tuple_lock_retry:
tuple->t_self = *tid;
result = heap_lock_tuple(relation, tuple, cid, mode, wait_policy,
--
2.50.1
From 057189b13e905377766f0e6c667c23849241708c Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
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
---
src/backend/executor/execTuples.c | 6 +++
src/test/regress/expected/gist.out | 62 ++++++++++++++++++++++++++++++
src/test/regress/sql/gist.sql | 48 +++++++++++++++++++++++
3 files changed, 116 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..e923fdbc296 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -463,3 +463,65 @@ 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_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);
+ ctid_not_found
+----------------
+ 0
+(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..53116f128a2 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -236,3 +236,51 @@ 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_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);
+
+-- 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 7ed9738b2a8d2471673fbff88418fcfe411842f2 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
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().
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 129d005f187..4bd52405d80 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -250,11 +250,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
