Re: Remove HeapTuple and Buffer dependency for predicate locking functions

On Wed, Jul 31, 2019 at 12:37 PM Ashwin Agrawal <aagrawal(at)pivotal(dot)io> wrote:

>
> On Wed, Jul 31, 2019 at 10:55 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> Hi,
>>
>> On 2019-07-31 10:42:50 -0700, Ashwin Agrawal wrote:
>> > On Tue, Jul 30, 2019 at 2:58 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
>> wrote:
>> >
>> > > On Tue, Jun 25, 2019 at 6:02 AM Andres Freund <andres(at)anarazel(dot)de>
>> wrote:
>> > > > > - 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.
>> > >
>> > > Thoughts on this Ashwin?
>> > >
>> >
>> > I think the only part its doing for sub-transaction is
>> > SubTransGetTopmostTransaction(xid). If xid passed to this function is
>> > already top most transaction which is case for zheap and zedstore, then
>> > there is no downside to keeping that code here in common place.
>>
>> Well, it's far from a cheap function. It'll do unnecessary on-disk
>> lookups in many cases. I'd call that quite a downside.
>>
>
> Okay, agree, its costly function and better to avoid the call if possible.
>
> Instead of moving the handling out of the function, how do feel about
> adding boolean isTopTransactionId argument to function
> CheckForSerializableConflictOut(). The AMs, which implicitly know, only
> pass top transaction Id to this function, can pass true and avoid the
> function call to SubTransGetTopmostTransaction(xid). With this
> subtransaction code remains in generic place and AMs intending to use it
> continue to leverage the common code, plus explicitly clarifies the
> behavior as well.
>

Added argument to function to make the subtransaction handling optional in
attached v2 of patch.