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

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-committers(at)lists(dot)postgresql(dot)org
Subject: Re: pgsql: Fix race condition in snapshot caching when 2PC is used.
Date: 2020-08-19 03:11:20
Message-ID: CAApHDvpYm5CO9AmryJ0+KXuBMDYw3EryP+ZrA_FNqeuqz966XA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

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

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Tom Lane 2020-08-19 03:31:04 Re: pgsql: Fix race condition in snapshot caching when 2PC is used.
Previous Message Andres Freund 2020-08-19 02:34:00 Re: prepared transaction isolation tests