From efdc7a5480293cfd8f57e9fe15ec7a062b7682d1 Mon Sep 17 00:00:00 2001 From: Greg Burd 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 Reported-by: Greg Burd Reviewed-by: Andres Freund Reviewed-by: Dilip Kumar Reviewed-by: Nikolay Samokhvalov Reviewed-by: Virender Singla 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