On Thu, Aug 1, 2019 at 2:36 AM Andres Freund <and...@anarazel.de> wrote: > > > > > > > > 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. > > Looking at the code as of master, we currently have: > > - PredicateLockTuple() calls SubTransGetTopmostTransaction() to figure > out a whether the tuple has been locked by the current > transaction. That check afaict just should be > TransactionIdIsCurrentTransactionId(), without all the other > stuff that's done today. > Yeah. this is the only part where predicate locking uses the subxids. Since, predicate locking always use the top xid, IMHO, it'll be good to make this api independent of subxids.
> TransactionIdIsCurrentTransactionId() imo ought to be optimized to > always check for the top level transactionid first - that's a good bet > today, but even moreso for the upcoming AMs that won't have separate > xids for subtransactions. Alternatively we shouldn't make that a > binary search for each subtrans level, but just have a small > simplehash hashtable for xids. A check for top transaction id first and usage of simple sound like good optimizations. But, I'm not sure whether these changes should be part of this patch or a separate one. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
