Re: Avoid using raw xmin of frozen tuples in logicalrep conflict detection

Hi again fellow hackers, may y'all stay happy and fresh.

On Mon, 9 Nov 2026 at 17:12, Osama Abdul Qader <
osamaabdulqaderpostgres(at)gmail(dot)com> wrote:

> Hi,
>
> On Mon, 9 Nov, 2026, 5:05 pm Zhijie Hou, <houzhijie22(at)gmail(dot)com> 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.