Hello Zhijie and Everyone On Fri, 6 Nov 2026 at 09:43, Zhijie Hou <[email protected]> wrote:
> Hi Andres, > > On Wed, Nov 4, 2026 at 09:33 PM Andres Freund wrote: > > On 2026-11-04 21:04:42 +0800, Zhijie Hou wrote: > >> The reason is that, GetTupleTransactionInfo() used slot_getsysattr() > with > >> MinTransactionIdAttributeNumber to fetch the local tuple's xmin. > >> However, heap_getsysattr() calls HeapTupleHeaderGetRawXmin(), which > >> returns the raw 32-bit t_xmin from the tuple header even when the tuple > >> is marked frozen (HEAP_XMIN_FROZEN). > >> > >> Once a tuple is frozen, its raw t_xmin is retained only for debugging > >> purposes; its commit timestamp entry may no longer be valid, or the > >> 32-bit XID may have wrapped around and been reassigned in a later epoch. > >> Using the raw xmin of a frozen tuple could therefore trigger spurious > >> update_origin_differs or delete_origin_differs conflicts, or report > >> bogus transaction ID, origin, and commit timestamp details for unique > >> constraint conflicts. > >> > >> Attaching a patch fixing this by fetching the tuple via > >> ExecFetchSlotHeapTuple() and calling HeapTupleHeaderGetXmin(), returning > >> InvalidTransactionId and skipping the commit timestamp lookup when the > tuple's > >> xmin is not a normal transaction ID. Also update > errdetail_apply_conflict() to > >> omit the transaction ID from unique-conflict detail messages when > localxmin is > >> invalid. > > > > That surely is not the right fix, it breaks any sense of abstraction > across > > table AMs. There's afaict no guarantee that the target table is heapam. > And > > even if there currently is, we shouldn't bake that in further. > > Thanks for pointing it out. I agree that conflict.c shouldn't be > reaching into BufferHeapTupleTableSlot or HeapTupleHeaderGetXmin() > directly. > > To avoid that, on master, it seems cleanest to handle this at the > TupleTableSlotOps level (alongside getsysattr and is_current_xact_tuple > in execTuples.c), for example, via an optional callback that returns the > effective xmin (HeapTupleHeaderGetXmin() for heap/buffer-heap slots, and > InvalidTransactionId when NULL for virtual/minimal slots or AMs that > don't store tuple xmin). > > TransactionId (*get_effective_xmin) (TupleTableSlot *slot); > > I also considered adding a callback to TableAmRoutine instead, but > wasn't sure if it's worth adding a TableAmRoutine entry when the tuple > is already in the slot and getsysattr / is_current_xact_tuple are on > TupleTableSlotOps. > > For back-patching to 18/19 where we can't change TupleTableSlotOps > without breaking ABI, GetTupleTransactionInfo() can branch on > TTS_IS_BUFFERTUPLE(localslot) || TTS_IS_HEAPTUPLE(localslot) to use > HeapTupleHeaderGetXmin() and fall back to slot_getsysattr() otherwise, > while still filtering out !TransactionIdIsNormal(xmin). > > What do you think ? > > -- > Best Regards, > Zhijie Hou > > > Thanks for the explanation and for providing the updated design direction. I've reviewed the v1 patch and the relevant code paths, including GetTupleTransactionInfo(), TupleTableSlotOps, and its implementations in execTuples.c. I agree that the patch addresses a real issue: using the raw xmin of a frozen tuple can expose an obsolete transaction ID. This can lead to misleading conflict detection and reporting, particularly when commit timestamp information is no longer available or the transaction ID has been reused. However, I also agree with Andres's concern that directly accessing BufferHeapTupleTableSlot or using HeapTupleHeaderGetXmin() inside conflict.c introduces a table access method abstraction problem. The conflict-handling code should not need to know the underlying tuple storage implementation. One possible approach for master would be to extend TupleTableSlotOps with a callback such as: `TransactionId (*get_effective_xmin) (TupleTableSlot *slot);` Each slot implementation could then provide the appropriate behavior: - Heap tuple and buffer heap tuple slots would return the effective xmin from the underlying tuple header. - Virtual and minimal tuple slots, or implementations that cannot provide a meaningful xmin, would return InvalidTransactionId. GetTupleTransactionInfo() could then obtain the effective xmin through this callback, check TransactionIdIsNormal(), and proceed with commit timestamp and replication origin lookup only when a valid transaction ID is available. This would keep the conflict-handling code independent of heap-specific details while making the semantics of retrieving an effective xmin explicit at the slot abstraction level. For backpatch branches, I understand that modifying TupleTableSlotOps may not be appropriate because of ABI compatibility concerns. We may therefore need a separate, version-specific solution that avoids introducing an ABI change while still handling frozen tuples correctly. I think this separates the underlying design issue from the immediate bug fix and provides a cleaner direction for master. What do you think about introducing get_effective_xmin as a slot operation for master? Best regards, Osama Abdul Qader
