On Wed, 2004-12-01 at 21:51 -0500, Bruce Momjian wrote:
> Neil, where are we on this? Should we add comments? Add a TODO? A patch?
I'm not sure what the right resolution is. As I said, I don't think it's
wise to apply a patch that could have a significant impact on
performance without (a) testing its performance effect and/or (b) having
any evidence that the problem it addresses actually effects anyone in
the real world. I'll try to run some benchmarks when I get a chance.
I wrote up most of a patch to implement the "wake up all shared wakers
on LWLockRelease()" behavior to see how that would change performance,
but the patch has a subtle bug in it that I can't seem to find (I've
attached it -- comments welcome).
Certainly if we decide to leave things as they are I think we ought to
document why the behavior is intentional, but I don't think we have
enough data to make that decision yet.
-Neil
--- src/backend/storage/lmgr/lwlock.c
+++ src/backend/storage/lmgr/lwlock.c
@@ -411,7 +411,7 @@
LWLockRelease(LWLockId lockid)
{
volatile LWLock *lock = LWLockArray + lockid;
- PGPROC *head;
+ PGPROC *release = NULL;
PGPROC *proc;
int i;
@@ -450,34 +450,67 @@
* any waiters if someone has already awakened waiters that haven't
* yet acquired the lock.
*/
- head = lock->head;
- if (head != NULL)
+ if (lock->head != NULL)
{
if (lock->exclusive == 0 && lock->shared == 0 && lock->releaseOK)
{
/*
- * Remove the to-be-awakened PGPROCs from the queue. If the
- * front waiter wants exclusive lock, awaken him only.
- * Otherwise awaken as many waiters as want shared access.
+ * Remove the to-be-awakened PGPROCs from the queue. If
+ * the front waiter wants an exclusive lock, awaken him
+ * only. Otherwise, awaken all the shared waiters in the
+ * queue.
*/
- proc = head;
- if (!proc->lwExclusive)
+ proc = lock->head;
+ if (proc->lwExclusive)
{
- while (proc->lwWaitLink != NULL &&
- !proc->lwWaitLink->lwExclusive)
+ lock->head = proc->lwWaitLink;
+ proc->lwWaitLink = NULL;
+ release = proc;
+ }
+ else
+ {
+ /*
+ * Walk through the wait queue. We form two linked
+ * lists: exclusive waiters, who will be the lock's
+ * new wait queue, and shared waiters, all of whom
+ * will be awaken.
+ */
+ PGPROC *exclusive_head = NULL;
+ PGPROC *exclusive_tail = NULL;
+ PGPROC *shared_head = proc;
+ PGPROC *shared_tail = proc;
+ proc = proc->lwWaitLink;
+ while (proc != NULL)
+ {
+ if (proc->lwExclusive)
+ {
+ if (!exclusive_head)
+ exclusive_head = exclusive_tail = proc;
+ else
+ {
+ exclusive_tail->lwWaitLink = proc;
+ exclusive_tail = proc;
+ }
+ }
+ else
+ {
+ shared_tail->lwWaitLink = proc;
+ shared_tail = proc;
+ }
proc = proc->lwWaitLink;
+ }
+
+ /* NULL terminate both lists */
+ shared_tail->lwWaitLink = NULL;
+ if (exclusive_tail)
+ exclusive_tail->lwWaitLink = NULL;
+ /* The exclusive list is the new wait queue */
+ lock->head = exclusive_head;
+ release = shared_head;
}
- /* proc is now the last PGPROC to be released */
- lock->head = proc->lwWaitLink;
- proc->lwWaitLink = NULL;
/* prevent additional wakeups until retryer gets to run */
lock->releaseOK = false;
}
- else
- {
- /* lock is still held, can't awaken anything */
- head = NULL;
- }
}
/* We are done updating shared state of the lock itself. */
@@ -486,11 +519,11 @@
/*
* Awaken any waiters I removed from the queue.
*/
- while (head != NULL)
+ while (release != NULL)
{
LOG_LWDEBUG("LWLockRelease", lockid, "release waiter");
- proc = head;
- head = proc->lwWaitLink;
+ proc = release;
+ release = proc->lwWaitLink;
proc->lwWaitLink = NULL;
proc->lwWaiting = false;
PGSemaphoreUnlock(&proc->sem);
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
