Re: Remove HeapTuple and Buffer dependency for predicate locking functions

On Wed, Jul 31, 2019 at 12:37 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:

>
> On Wed, Jul 31, 2019 at 10:55 AM Andres Freund <andres@anarazel.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.munro@gmail.com>
>> wrote:
>> >
>> > > On Tue, Jun 25, 2019 at 6:02 AM Andres Freund <andres@anarazel.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.