On Tue, Nov 15, 2011 at 1:40 PM, Simon Riggs <si...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers