On Thu, Dec 25, 2014 at 11:49 AM, Andres Freund <andres@anarazel.de> wrote:
> Convert the PGPROC->lwWaitLink list into a dlist instead of open coding it.
>
> Besides being shorter and much easier to read it changes the logic in
> LWLockRelease() to release all shared lockers when waking up any. This
> can yield some significant performance improvements - and the fairness
> isn't really much worse than before, as we always allowed new shared
> lockers to jump the queue.
Two different colleagues of mine have recently and independently of
each other discovered a major down side of this commit: it breaks the
ability to put an LWLock into a DSM segment, which was a principle
goal of ea9df812d8502fff74e7bc37d61bdc7d66d77a7f. When that commit
went in, an LWLock only needed to store pointers to PGPROC structure,
which don't move even though the LWLock itself might. But the dlist
implementation uses a *circular* doubly linked list, which means that
there are not only pointers to the PGPROC in play but also pointers
back to the LWLock itself, and that breaks when DSM segments don't map
in at the same address in all cooperating processes.
I don't know exactly what to do about this, but I think it's a
problem. In one of the two cases, an LWLock wasn't really the right
data structure anyway; what was really needed was a condition
variable. But in the other case, the thing that is needed is
precisely an LWLock. Parallel sequential scan doesn't run afoul of
this restriction because we can get by with spinlocks, but as we
develop more complicated parallel operations (parallel index scan,
parallel bitmap index scan, parallel hash) I think we really need to
have other synchronization primitives available, including LWLocks.
They are a fundamental part of the PostgreSQL synchronization model,
and it's hard to write complex code without them.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
