On Wed, 19 Aug 2020 at 12:37, Andres Freund <[email protected]> wrote: > > Hi, > > On 2020-08-18 19:55:50 -0400, Tom Lane wrote: > > > I'm inclined to just make ClearTransaction take an exclusive lock - the > > > rest of the 2PC operations are so heavyweight that I can't imagine > > > making a difference. When I tested the locking changes in > > > ProcArrayAdd()/Remove() the more heavyweight locking wasn't at all > > > visible. > > > > I was wondering if it'd be sensible to convert that counter into an > > atomic variable. That's not real clear, but worth thinking about. > > I had it that way originally, but because xactCompletionCount otherwise > needs to be modified with ProcArrayLock exclusively held, I moved it to > a plain variable. It'd be a bad deal to actually modify it atomically > unless we got something out of that ([1]), because that'd increase the > number of atomic operations for commits, something we surely don't want. > > We could make the locking rules be that modifications with a shared > ProcArrayLock have to be done with an atomic op, and operations with > ProcArrayLock held exclusively may do it without an atomic op. But that > seems a bit weird.
Couldn't it be done by creating two inline functions, one to call to atomically increment and the other to just increment? Can backup that the correct version of the function is being called with an Assert(LWLockHeldByMeInMode(ProcArrayLock, ...)); The weirdness of doing var++ on an atomic variable can be removed by mentioning why it's ok in a comment in the inline function. David
