Re: BUG #17462: Invalid memory access in heapam_tuple_lock

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2022-04-11 15:59:30 -0400, Tom Lane wrote:
>> Yeah, that could work. You want to draft a patch, or shall I?

> If you would, I'd appreciate it.

I abandoned that idea after noticing that heapam_tuple_lock needs
not only the tuple's xmin and ctid, but usually also the result of
HeapTupleHeaderGetUpdateXid, which is expensive (it can require a
multixact lookup). I do not think it's a great idea to expect
HeapTupleSatisfiesDirty to compute that. So the attached patch goes back
to my original idea of reverting the removal of the keep_buf argument.
This worked out pretty cleanly, with minimal code changes. In HEAD,
we might want to avoid the extra heap_fetch_extended layer by
adding that argument back to heap_fetch, but having the extra layer
avoids an API break for the back branches.

As written here, even though there's no obvious API break, there is
a subtle change: heap_fetch will now return tuple->t_data = NULL after
a snapshot failure. I'm of two minds whether to back-patch that part
(not doing so would require more changes in heapam_tuple_lock, though).
I'm concerned that some caller might be using that legitimately, say
by testing for pointer nullness without actually dereferencing it.
And even if they are unsafely dereferencing it, I'm not sure people
would thank us for converting a subtle low-probability failure into a
subtle higher-probability one. Thoughts?

regards, tom lane