Re: Remove HeapTuple and Buffer dependency for predicate locking functions

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.