On 17/12/2025 17:42, Heikki Linnakangas wrote:
> On 17/12/2025 16:51, Jasper Smit wrote:
>>> Thanks! That's not quite correct though. This is very subtle, but the
>>> 'tuple' argument to heap_lock_updated_tuple() points to the buffer
>>> holding the original tuple. So doing
>>> HeapTupleHeaderGetUpdateXid(tuple->t_data) reads the current xmax of the
>>> tuple, which can now be different than what it was earlier, i.e.
>>> different from the xwait that we waited for. It's important that the
>>> 'ctid' and 'priorXmax' that we use to follow the chain are read
>>> together.
>>
>> Oops, you are right, I was too quick, it has already been quite some
>> time since i was dealing with this problem initially.
>>
>> I see you moved the check ItemPointerEquals(&tuple->t_self, ctid) out
>> to heap_lock_tuple()
>> but isn't this redundant in the check `if (follow_updates && updated
>> && !ItemPointerEquals(&tuple->t_self, &t_ctid))` if `updated` is
>> already true in this condition?
>
> Hmm, I don't think so, HEAP_KEYS_UPDATED is also set on deleted tuples.
Ah, but this codepath is taken when HEAP_KEYS_UPDATED is *not* set. I
got that backwards. So I agree the ItemPointerEquals(&tuple->t_self,
ctid) check is redundant.
This is so subtle that I'm inclined to nevertheless keep it, at least in
backbranches. Just in case we're missing something. In master, we could
perhaps add an assertion for it.
Here's a new patch version. I worked some more on the test, making it
less sensitive to the tuple layout. It should now pass on 32-bit systems
and with different block sizes.
- Heikki
