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.
