Re: FlexLocks

On Tue, Nov 15, 2011 at 1:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Which is the same locking avoidance technique we already use for sync
> rep and for the new group commit patch.

Yep...

> I've been saying for some time that we should use the same technique
> for ProcArray and clog also, so we only need to queue once rather than
> queue three times at end of each transaction.
>
> I'm not really enthused by the idea of completely rewriting lwlocks
> for this. Seems like specialised code is likely to be best, as well as
> having less collateral damage.

Well, the problem that I have with that is that we're going to end up
with a lot of specialized code, particularly around error recovery.
This code doesn't remove the need for ProcArrayLock to be taken in
exclusive mode, and I don't think there IS any easy way to remove the
need for that to happen sometimes. So we have to deal with the
possibility that an ERROR might occur while we hold the lock, which
means we have to release the lock and clean up our state. That means
every place that has a call to LWLockReleaseAll() will now also need
to cleanup ProperlyCleanUpProcArrayLockStuff(). And the next time we
need to add some kind of specialized lock, we'll need to do the same
thing again. It seems to me that that rapidly gets unmanageable, not
to mention *slow*. We need some kind of common infrastructure for
releasing locks, and this is an attempt to create such a thing. I'm
not opposed to doing it some other way, but I think doing each one as
a one-off isn't going to work out very well.

Also, in this particular case, I really do want shared and exclusive
locks on ProcArrayLock to stick around; I just want one additional
operation as well. It's a lot less duplicated code to do that this
way than it is to write something from scratch. The FlexLock patch
may look big, but it's mostly renaming and rearranging; it's really
not adding much code.

> With that in mind, should we try to fuse the group commit with the
> procarraylock approach, so we just queue once and get woken when all
> the activities have been handled? If the first woken proc performs the
> actions then wakes people further down the queue it could work quite
> well.

Well, there's too much work there to use the same approach I took
here: we can't very well hold onto the LWLock spinlock while flushing
WAL or waiting for synchronous replication. Fusing together some
parts of the commit sequence might be the right approach (I don't
know), but honestly my gut feeling is that the first thing we need to
do is go in the opposite direction and break up WALInsertLock into
multiple locks that allow better parallelization of WAL insertion. Of
course if someone finds a way to fuse the whole commit sequence
together in some way that improves performance, fantastic, but having
tried a lot of things before I came up with this approach, I'm a bit
reluctant to abandon it in favor of an approach that hasn't been coded
or tested yet. I think we should pursue this approach for now, and we
can always revise it later if someone comes up with something even
better. As a practical matter, the test results show that with these
patches, ProcArrayLock is NOT a bottleneck at 32 cores, which seems
like enough reason to be pretty happy with it, modulo implementation
details.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company