| From: | "Greg Burd" <greg(at)burd(dot)me> |
|---|---|
| To: | "Virender Singla" <virender(dot)cse(at)gmail(dot)com> |
| Cc: | "PostgreSQL Hackers" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "Andres Freund" <andres(at)anarazel(dot)de>, "Michael Paquier" <michael(at)paquier(dot)xyz>, "Dilip Kumar" <dilipbalaut(at)gmail(dot)com> |
| Subject: | Re: [PATCH] Corruption Issue: Fix missing tts_tid in ExecForceStoreHeapTuple |
| Date: | 2026-09-14 13:19:21 |
| Message-ID: | 9d79b7ff-1267-4a2e-b441-dddbc7c81781@app.fastmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Sep 14, 2026, at 8:31 AM, Dilip Kumar wrote:
> On Mon, Sep 14, 2026 at 5:47 PM Burd, Greg <greg(at)burd(dot)me> wrote:
>>
>>
>>
>> > On Sep 14, 2026, at 8:10 AM, Virender Singla <virender(dot)cse(at)gmail(dot)com> 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,
-greg
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Restore-slot-tts_tid-in-ExecForceStoreHeapTuple-s.patch | text/x-patch | 8.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Burd, Greg | 2026-09-14 13:26:24 | Re: ExecForceStoreHeapTuple() loses tts_tid, so ORDER BY-op index scans project an invalid ctid |
| Previous Message | Lucas Jeffrey | 2026-09-14 12:56:31 | Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) |