Re: pgsql: Fix race condition in snapshot caching when 2PC is used.

On Wed, 19 Aug 2020 at 12:37, Andres Freund <andres(at)anarazel(dot)de> 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