On Thursday, October 1st, 2026 at 12:58 PM, Andres Freund <[email protected]>
wrote:
> Hi,
Hello Andres, thanks for looking into this.
> 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.
Yep, done, addresses on everything, both Discussion links, and Reviewed-by for
you, Dilip, Nik and Virender in v7 attached. What's the convention when someone
both reports an issue and co-authors the patch? I added myself and Virender
as reporters, but maybe we're also co-authors too? Still learning this... :)
I'm dropping the attempts at adding asserts in other paths mainly because I'm
not sure of the value but also because while testing I ran into b8b94ea129f:
For amcanreorderby scans the nodeIndexscan.c's reorder queue holds
heap tuples, but the underlying table likely does not.
For such an AM the scan slot isn't a buffer slot, so reorderqueue_push() goes
ExecCopySlotHeapTuple() -> tts_virtual_copy_heap_tuple() -> heap_form_tuple(),
which does ItemPointerSetInvalid(&tuple->t_self). The queued tuple therefore
has no TID to begin with. 0001 stores that invalid TID and the assertion then
fires on a slot that is behaving correctly. heap_copytuple() preserves t_self,
which is why heap never hits this.
> On 2026-10-01 10:16:05 -0400, Andres Freund wrote:
> > On 2026-09-29 16:31:39 +0000, Greg Burd wrote:
> > > @@ -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.
>
> None of the copyslots maintains it properly either. Some just don't set it,
> others will *sometimes* infer it from the source tuple
> (e.g. tts_buffer_heap_copyslot, if the source slot is a buffer slot). Nor is
> tts_tableOid consistently maintained as part of a copy.
>
> That seems to currently not really be observable, because afaict we always
> push down the projection of ctid, tableoid accesses down to the scan level,
> where the slot is populated "originally" (if you assume nodeModifyTable.c is a
> scan node, at least).
>
> Which I guess is an argument to not consider that aspect a bug.
Agreed.
> > 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.
>
> AI did find a path, see attached tid2.sql, note the wrong tid for
> INSERT INTO parent_rows VALUES (2) RETURNING a, ctid;
>
> Seems we should just move
> slot->tts_tid = tuple->t_self;
> to the end of ExecForceStoreHeapTuple()?
I took a copy of the TID at function start and use that later to avoid a case
where the tuple might have bee free'd.
> After looking at tid2.sql I played around trying to see whether it'd make it
> possible to get an invalid tid for an update and was rather horrified to find
> out that cross-partition updates involving ctids just silently cause data to
> be inconsistent with the partition constraints :(.
Yikes, good find that sounds worse than anything in this thread. The
postgres_fdw side looks like a separate patch, and I don't think it
should hold up the backpatchable fix. I'll take a look at it
separately if nobody else is already on it and start a new thread.
> Greetings,
>
> Andres Freund
v7/0001 the fix, now maintaining the TID for all slot types, plus the
regression test. This is the piece that wants backpatching.
On backpatching 0001: applies as-is to 18, 17, 16 and 15. On 14 the C
hunk applies with an offset but the test hunks don't, gist.sql has grown
since. The test block is self-contained, so appending it to the end of
14's gist.sql and gist.out is enough IMO.
best.
-greg
From efdc7a5480293cfd8f57e9fe15ec7a062b7682d1 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 14 Sep 2026 12:58:22 -0400
Subject: [PATCH v7] Maintain tts_tid in ExecForceStoreHeapTuple()
ExecForceStoreHeapTuple() clears the target slot, which resets tts_tid,
and then stores the tuple without restoring it. Both
tts_heap_store_tuple() and tts_buffer_heap_store_tuple() assign
slot->tts_tid = tuple->t_self and so should this.
The oversight 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 issue surfaces when 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 (InvalidBlockNumber). 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 fix is to set the TID once at the end of the function so every slot
type gets it, rather than only the TTS_IS_BUFFERTUPLE branch. The TID is
read from a copy taken on entry because the tuple may have been freed.
Note that this does not make every path project a valid ctid. A tuple
built by heap_form_tuple() carries an invalid t_self, so a caller that
hands over such a tuple still stores an invalid TID; postgres_fdw's
tuple-routed INSERT ... RETURNING ctid is one such case, because the
remote query does not ask for ctid and make_tuple_from_result_row()
only fills t_self in when it did. That is a separate problem in the
caller, not in this function, which can only advertise the TID it is
given.
The test added to gist 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 check's qual into the scan. Note
that the first tuple is returned without being queued, so a check at
LIMIT 1 passes even without the fix.
Reported-by: Virender Singla <[email protected]>
Reported-by: Greg Burd <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Dilip Kumar <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Virender Singla <[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 | 42 ++++++++++++++++++++++++++++++
src/test/regress/sql/gist.sql | 36 +++++++++++++++++++++++++
3 files changed, 86 insertions(+)
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index b8e8f52c64c..ef508921cf6 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -1752,6 +1752,8 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
TupleTableSlot *slot,
bool shouldFree)
{
+ ItemPointerData tid = tuple->t_self;
+
if (TTS_IS_HEAPTUPLE(slot))
{
ExecStoreHeapTuple(tuple, slot, shouldFree);
@@ -1784,6 +1786,12 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
pfree(tuple);
}
}
+
+ /*
+ * Store the TID from the local copy taken on entry since the stores above
+ * may have freed the tuple.
+ */
+ slot->tts_tid = tid;
}
/*
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index ac79f94aa80..46b4d1a4952 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -463,3 +463,45 @@ 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;
+-- 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)
+
+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..ed80e5d7c1b 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -236,3 +236,39 @@ 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;
+
+-- 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);
+
+reset enable_seqscan;
+drop table gist_knn_ctid_res;
+drop table gist_knn_ctid;
--
2.50.1
