Hello.
v8 attached, code/fix is unchanged from v7 but just some cleanup of things
I'd overlooked or misunderstood so a new commit message and some additional
tests.
First, the tid2.sql case *is* fixed by this patch. I said otherwise, and
I'm sorry for the confusion. I'd tested against the wrong build, and then
misread EXPLAIN VERBOSE: the routed INSERT shows no "Remote SQL ...
RETURNING", which I took to mean ctid wasn't being requested. That remote
statement is built at execution time in postgresBeginForeignInsert(),
which passes ri_returningList through to deparseReturningList(), so ctid
is requested and make_tuple_from_result_row() does fill in t_self. What
lost it was the else (virtual-slot) branch of ExecForceStoreHeapTuple()
not setting tts_tid, which is exactly what moving the assignment to the
end (as you had suggested Andres) fixes.
Testing against a fresh build:
INSERT INTO parent_rows VALUES (2) RETURNING a, ctid;
v8: 2 | (0,2)fixes
execTuples.c reverted: 2 | (4294967295,0)
v8 drops the paragraph in v7's commit message that claimed this was a
separate postgres_fdw problem, and adds your case as a regression test
in postgres_fdw.sql, next to the existing insert tuple routing tests.
It inserts into itrtest and checks every returned ctid against
loct1/loct2, so as not to depend on page layout.
While writing it I found that only one of the two partitions in that
test shows the bug: remp1, whose columns match the parent, gets a virtual
result slot. remp2 declares (b, a), so its insert goes through a tuple
conversion map and a heap slot, which was already correct. The test
therefore reports one missing ctid rather than two without the fix.
Second, in my last mail I answered your cross-partition update comment
with "The postgres_fdw side looks like a separate patch". That attached
the FDW case to the wrong issue. As far as I can tell they're unrelated:
the FDW case was this bug, while
> cross-partition updates involving ctids just silently cause data to be
> inconsistent with the partition constraints :(.
is something else. I tried a few obvious shapes, a ctid-keyed UPDATE that
moves the row, a ctid-only predicate that matches (0,1) in two
partitions, and a ctid taken from a subquery against another partition,
and they all came out consistent with the partition constraints. So I
haven't found your case. Could you share what you ran? I'm happy to work
on it in a new thread.
On attribution, I'll answer my own question from last time, I've used
Co-authored-by: me and Virender, and Reported-by: you for the FDW
path. Tell me if you'd rather it were different.
Backpatching: v8 applies as-is to master and 15 through 18. On 14 the
execTuples.c hunk applies, but both test hunks need placing by hand
because gist.sql and postgres_fdw.sql have grown since. Both blocks are
self-contained and go in unchanged. I built 14 with --enable-cassert and
the placed postgres_fdw test passes with the fix and fails without it
(ctid_not_found 0 -> 1), the same as on master.
Apologies for rushing out the last email.
best.
-greg
From 6225daa836b9926b5c0cb187f14dc8c7f1b7baf0 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 14 Sep 2026 12:58:22 -0400
Subject: [PATCH v8] 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.
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 <[email protected]>
Co-authored-by: Greg Burd <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Dilip Kumar <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[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
---
.../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 739f43af7bb..291130ebcba 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9926,6 +9926,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;
-- MERGE ought to fail cleanly
merge into itrtest using (select 1, 'foo') as source on (true)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f1ca3204382..3679bad53da 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2959,6 +2959,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;
+
-- MERGE ought to fail cleanly
merge into itrtest using (select 1, 'foo') as source on (true)
when matched then do nothing;
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
