Re: Remove HeapTuple and Buffer dependency for predicate locking functions

Hi,

On 2019-06-24 10:41:06 -0700, Ashwin Agrawal wrote:
> Proposing following changes to make predicate locking and checking
> functions generic and remove dependency on HeapTuple and Heap AM. We
> made these changes to help with Zedstore. I think the changes should
> help Zheap and other AMs in general.

Indeed.

> - Change PredicateLockTuple() to PredicateLockTID(). So, instead of
> passing HeapTuple to it, just pass ItemPointer and tuple insert
> transaction id if known. This was also discussed earlier in [1],
> don't think was done in that context but would be helpful in future
> if such requirement comes up as well.

Right.

> - CheckForSerializableConflictIn() take blocknum instead of
> buffer. Currently, the function anyways does nothing with the buffer
> just needs blocknum. Also, helps to decouple dependency on buffer as
> not all AMs may have one to one notion between blocknum and single
> buffer. Like for zedstore, tuple is stored across individual column
> buffers. So, wish to have way to lock not physical buffer but
> logical blocknum.

Hm. I wonder if we somehow ought to generalize the granularity scheme
for predicate locks to not be tuple/page/relation. But even if, that's
probably a separate patch.

> - CheckForSerializableConflictOut() no more takes HeapTuple nor
> buffer, instead just takes xid. Push heap specific parts from
> CheckForSerializableConflictOut() into its own function
> HeapCheckForSerializableConflictOut() which calls
> CheckForSerializableConflictOut(). The alternative option could be
> CheckForSerializableConflictOut() take callback function and
> callback arguments, which gets called if required after performing
> prechecks. Though currently I fell AM having its own wrapper to
> perform AM specific task and then calling
> CheckForSerializableConflictOut() is fine.

I think it's right to move the xid handling out of
CheckForSerializableConflictOut(). But I think we also ought to move the
subtransaction handling out of the function - e.g. zheap doesn't
want/need that.

> Attaching patch which makes these changes.

Please make sure that there's a CF entry for this (I'm in a plane with a
super slow connection, otherwise I'd check).

Greetings,

Andres Freund