On Mon, Sep 14, 2026, at 8:31 AM, Dilip Kumar wrote:
> On Mon, Sep 14, 2026 at 5:47 PM Burd, Greg <[email protected]> wrote:
>>
>>
>>
>> > On Sep 14, 2026, at 8:10 AM, Virender Singla <[email protected]>
>> > 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,
-gregFrom 075c4e5dc0c7ae2f915d6ec742a94e2414167fdc Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Tue, 8 Sep 2026 12:35:25 -0400
Subject: [PATCH v3] Restore slot->tts_tid in ExecForceStoreHeapTuple's
buffer-slot path
ExecForceStoreHeapTuple() loses the tuple's item pointer when the target
slot is a TTS_IS_BUFFERTUPLE slot. That branch calls ExecClearTuple(),
whose tts_buffer_heap_clear() does ItemPointerSetInvalid(&slot->tts_tid),
then installs bslot->base.tuple = heap_copytuple(tuple) but never copies
tuple->t_self back into slot->tts_tid. The slot is therefore left
advertising InvalidBlockNumber.
The sibling routine ExecStoreHeapTuple() -> tts_heap_store_tuple() does
set slot->tts_tid = tuple->t_self, so the omission looks like a plain
asymmetry rather than an intentional choice.
This is user-visible because slot_getsysattr() answers
SelfItemPointerAttributeNumber straight out of slot->tts_tid. Any plan
that re-stores a heap tuple into a buffer slot through
ExecForceStoreHeapTuple() and then projects ctid gets (4294967295,0).
nodeIndexscan.c's reorder queue is one such path: reorderqueue_pop()
hands the palloc'd copy to ExecForceStoreHeapTuple(). So for any index
AM that sets xs_recheckorderby = true, every tuple that passes through
the reorder queue projects the invalid-tid sentinel instead of its real
heap tid, even though the AM set xs_heaptid correctly (which is why the
row data itself is right and only the ctid system column is wrong).
The consequences range from silently wrong to a crash:
- ctid-keyed dedup and UPDATE ... WHERE ctid = ... silently match the
one row that was returned without queueing, instead of all of them.
No error is raised.
- With row locking, the invalid tid reaches heap_lock_tuple(), which
asserts in ItemPointerIsValid(). In a non-assert build there is no
assert to catch it and InvalidBlockNumber == P_NEW, so ReadBuffer()
extends the relation on disk before failing with "attempted to lock
invisible tuple", leaving an uninitialized block behind that makes
later sequential scans fail with "invalid page in block N".
The regression test uses core GiST only: thin diagonal triangles, so the
bounding-box distance strictly under-estimates the true polygon
distance, gist_poly_consistent sets recheck, was_exact comes out false
and the tuples are pushed to the reorder queue. Note that the first
tuple is returned directly without queueing and so keeps its real ctid;
a check at LIMIT 1 therefore passes even on unpatched code, which is why
the test looks at five rows.
Present in all supported branches (REL_14_STABLE .. master); the
affected code in ExecForceStoreHeapTuple() is byte-identical across
them.
Co-authored-by: Virender Singla <[email protected]>
Co-authored-by: Greg Burd <[email protected]>
Reviewed-by: Dilip Kumar <[email protected]>
Discussion: https://postgr.es/m/CAM6Zo8wZOLnCWRO_tuuXVX9J4N4JN6GsEnk8WJtT0%3D_0zy-1dw%40mail.gmail.com
Discussion: https://postgr.es/m/0498c10f-839b-4f68-9994-c29b454e55a4%40app.fastmail.com
Backpatch-through: 14
---
src/backend/executor/execTuples.c | 8 +++++
src/test/regress/expected/gist.out | 54 ++++++++++++++++++++++++++++++
src/test/regress/sql/gist.sql | 38 +++++++++++++++++++++
3 files changed, 100 insertions(+)
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index b8e8f52c64c..f14c6d6be07 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -1768,6 +1768,14 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
slot->tts_flags |= TTS_FLAG_SHOULDFREE;
MemoryContextSwitchTo(oldContext);
+ /*
+ * ExecClearTuple() above invalidated tts_tid; restore it from the
+ * tuple so that projecting ctid (slot_getsysattr() reads tts_tid)
+ * yields the real heap tid rather than InvalidBlockNumber. This
+ * matches what tts_heap_store_tuple() does for heap slots.
+ */
+ 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..66817c303c5 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -423,6 +423,60 @@ select lower(r) = repeat('7', 200)::numeric as lower_ok,
(1 row)
drop table gist_ios_tupdesc;
+-- 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, ('((' || i*10 || ',0),(' || (i*10+9) || ',9),('
+ || (i*10+9) || ',0))')::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;
+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;
+ 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)
+
+-- and row locking must not be handed the invalid tid, which asserts in
+-- ItemPointerIsValid() (or, in a non-assert build, tries to lock
+-- InvalidBlockNumber == P_NEW and extends the relation on disk)
+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;
+drop table gist_knn_ctid;
-- test deletion of LP_DEAD-marked index tuples
create table gist_prune_tbl (k int, p point);
create index gist_prune_tbl_p_index on gist_prune_tbl using gist (p);
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql
index 57dcc082450..72c0764f499 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -198,6 +198,44 @@ select lower(r) = repeat('7', 200)::numeric as lower_ok,
drop table gist_ios_tupdesc;
+-- 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, ('((' || i*10 || ',0),(' || (i*10+9) || ',9),('
+ || (i*10+9) || ',0))')::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;
+
+explain (costs off)
+select ctid, 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 (select ctid from gist_knn_ctid order by p <-> point(100,4) limit 5) s
+where ctid = '(4294967295,0)'::tid;
+
+-- 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;
+
+-- and row locking must not be handed the invalid tid, which asserts in
+-- ItemPointerIsValid() (or, in a non-assert build, tries to lock
+-- InvalidBlockNumber == P_NEW and extends the relation on disk)
+begin;
+select count(*) as locked
+from (select id from gist_knn_ctid order by p <-> point(100,4) limit 5
+ for update) s;
+rollback;
+
+drop table gist_knn_ctid;
+
-- test deletion of LP_DEAD-marked index tuples
create table gist_prune_tbl (k int, p point);
create index gist_prune_tbl_p_index on gist_prune_tbl using gist (p);
--
2.50.1
