Re: Visibility bug in tuple lock

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