From acd432d1805127b5185cf2afde03ee5a660bd720 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 14 Sep 2026 12:58:22 -0400 Subject: [PATCH v9 REL_14] 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(), for instance through SELECT ... FOR UPDATE, damages the table. InvalidBlockNumber equals P_NEW, so ReadBuffer() extends the relation by a zeroed page. Offset 0 then indexes the line pointer before the first one, which reads as zero, so the tuple header is taken to start at byte 0 of the new page, and HeapTupleSatisfiesUpdate() sets HEAP_XMIN_INVALID in it before the lock fails with "attempted to lock invisible tuple". That hint bit leaves the page neither all-zeroes nor validly initialized, so once it has been written out and read back, for instance after a checkpoint and restart, scans and VACUUM of the table fail with "invalid page in block N". 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. This also covers the virtual-slot branch, which previously did not set tts_tid at all. postgres_fdw stores the tuple returned by a remote INSERT ... RETURNING through ExecForceStoreHeapTuple(), so an insert routed through a partitioned table into a foreign-table partition whose result slot is virtual returned (4294967295,0) for ctid, even though the remote side had sent back the real one. 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. The postgres_fdw test inserts through a partitioned table into foreign-table partitions and checks that every returned ctid matches a row in the remote table. Only the partition whose columns match the parent's gets a virtual result slot; the other goes through a tuple conversion map and a heap slot, which was already correct, so the test reports one missing row rather than two without the fix. Co-authored-by: Virender Singla Co-authored-by: Greg Burd Reviewed-by: Andres Freund Reviewed-by: Dilip Kumar Reviewed-by: Nikolay Samokhvalov Tested-by: Shihao Zhong 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 --- .../postgres_fdw/expected/postgres_fdw.out | 19 +++++++++ contrib/postgres_fdw/sql/postgres_fdw.sql | 16 +++++++ src/backend/executor/execTuples.c | 8 ++++ src/test/regress/expected/gist.out | 42 +++++++++++++++++++ src/test/regress/sql/gist.sql | 36 ++++++++++++++++ 5 files changed, 121 insertions(+) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 66c74f55a65..49e349fb573 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8352,6 +8352,25 @@ select tableoid::regclass, * FROM remp2; remp2 | test2 | 2 (3 rows) +delete from itrtest; +-- The ctid returned by a routed insert must be the row's real ctid on the +-- remote side. A foreign-table partition uses a virtual slot, and +-- ExecForceStoreHeapTuple() used to leave tts_tid invalid on that path, so +-- RETURNING ctid projected (4294967295,0). Compare against the remote table +-- rather than printing the ctid, which depends on the remote page layout. +create temp table itrtest_ctid (a int, c tid); +with ins as (insert into itrtest values (1, 'foo'), (2, 'bar') + returning a, ctid) +insert into itrtest_ctid select a, ctid from ins; +select count(*) as ctid_not_found from itrtest_ctid r +where not exists (select 1 from loct1 l where l.ctid = r.c and l.a = r.a) + and not exists (select 1 from loct2 l where l.ctid = r.c and l.a = r.a); + ctid_not_found +---------------- + 0 +(1 row) + +drop table itrtest_ctid; delete from itrtest; create unique index loct1_idx on loct1 (a); -- DO NOTHING without an inference specification is supported diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index af286d9c33d..b18125b385d 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2281,6 +2281,22 @@ select tableoid::regclass, * FROM remp2; delete from itrtest; +-- The ctid returned by a routed insert must be the row's real ctid on the +-- remote side. A foreign-table partition uses a virtual slot, and +-- ExecForceStoreHeapTuple() used to leave tts_tid invalid on that path, so +-- RETURNING ctid projected (4294967295,0). Compare against the remote table +-- rather than printing the ctid, which depends on the remote page layout. +create temp table itrtest_ctid (a int, c tid); +with ins as (insert into itrtest values (1, 'foo'), (2, 'bar') + returning a, ctid) +insert into itrtest_ctid select a, ctid from ins; +select count(*) as ctid_not_found from itrtest_ctid r +where not exists (select 1 from loct1 l where l.ctid = r.c and l.a = r.a) + and not exists (select 1 from loct2 l where l.ctid = r.c and l.a = r.a); +drop table itrtest_ctid; + +delete from itrtest; + create unique index loct1_idx on loct1 (a); -- DO NOTHING without an inference specification is supported diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 5f6d92e242c..0c80e186f1d 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -1470,6 +1470,8 @@ ExecForceStoreHeapTuple(HeapTuple tuple, TupleTableSlot *slot, bool shouldFree) { + ItemPointerData tid = tuple->t_self; + if (TTS_IS_HEAPTUPLE(slot)) { ExecStoreHeapTuple(tuple, slot, shouldFree); @@ -1502,6 +1504,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 7fdab88dcea..6a040ddb132 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -454,3 +454,45 @@ reset enable_seqscan; reset enable_bitmapscan; reset enable_indexonlyscan; 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 f6a05850c9f..aa319608498 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -225,3 +225,39 @@ reset enable_bitmapscan; reset enable_indexonlyscan; 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