Hi again fellow hackers, may y'all stay happy and fresh. On Mon, 9 Nov 2026 at 17:12, Osama Abdul Qader < [email protected]> wrote:
> Hi, > > On Mon, 9 Nov, 2026, 5:05 pm Zhijie Hou, <[email protected]> wrote: > >> Hi, >> >> On Fri, Nov 6, 2026 at 12:13 PM Zhijie Hou wrote: >> > Hi Andres, >> > >> > On Wed, Nov 4, 2026 at 09:33 PM Andres Freund wrote: >> > > 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); >> >> Following this, I am attaching two patches implementing the approaches >> discussed above for reference: >> >> v2-0001-Fix-logical-xx.patch adds an optional get_effective_xmin >> callback to TupleTableSlotOps (alongside getsysattr and >> is_current_xact_tuple). Heap and buffer-heap slots implement it via >> HeapTupleHeaderGetXmin(), while slots that do not provide this >> information leave the callback unset so slot_get_effective_xmin() >> returns InvalidTransactionId. >> >> nocfbot-v2-0001-Fix-logical-xx.patch avoids adding a new >> TupleTableSlotOps callback (which is also needed for back-patching to >> 18) by checking TTS_IS_BUFFERTUPLE(localslot) || >> TTS_IS_HEAPTUPLE(localslot) in GetTupleTransactionInfo() to use >> HeapTupleHeaderGetXmin(), and falling back to slot_getsysattr() for >> other slot types. >> >> What do you think ? >> >> -- >> Best Regards, >> Zhijie Hou > > > I'm open to review the new patches, I'll reply again after reviewing it, > not only that I'm also open to fix the issues that comes after reviewing > it. > > I regret for the inconvenience I caused earlier, I assure to everyone here > that this won't happen again. > > With Regards, > Osama Abdul Qader. > I've finished testing both patch alternatives for the frozen tuple xmin issue. I tested the following versions independently: 1. The callback-based v2 patch (get_effective_xmin). 2. The no-callback alternative. 3. The unpatched baseline. The unpatched baseline reproduced the original issue. In particular, UPDATE and DELETE operations against frozen tuples triggered false update_origin_differs and delete_origin_differs conflicts. I also verified the unique-conflict reporting behaviour. With the patched version, the conflict detail still identifies the unique index, conflicting key and local row, but no longer attributes a misleading transaction ID and timestamp to the frozen tuple. *Code review observation* For the callback-based approach, I think adding get_effective_xmin to TupleTableSlotOps is a cleaner abstraction. The caller requests the effective xmin through the slot interface without needing to know the underlying tuple representation. For the no-callback alternative, I inspected the TTS_IS_HEAPTUPLE and TTS_IS_BUFFERTUPLE checks, along with ExecFetchSlotHeapTuple() and the corresponding get_heap_tuple implementations. The heap-slot handling appears correct, including the shouldFree assertion. My main architectural question concerns the no-callback alternative: conflict.c now explicitly handles the two built-in heap slot types and falls back to slot_getsysattr() for other slot types. Is this considered an acceptable compromise for stable-branch backpatching, while the callback-based approach is preferred for master? I haven't identified a concrete correctness defect in either implementation from the paths I've reviewed so far. I'd appreciate your thoughts on the preferred approach and whether there are any additional cases you think should be covered. With Regards, Osama Abdul Qader.
