On 2014-03-07 17:54:32 +0200, Heikki Linnakangas wrote: > So there are some unexplained differences there, but based on these results, > I'm still OK with committing the patch.
So, I am looking at this right now. I think there are some minor things I'd like to see addressed: 1) I think there needs to be a good sized comment explaining why WaitXLogInsertionsToFinish() isn't racy due to the unlocked read at the beginning of LWLockWait(). I think it's safe because we're reading Insert->CurrBytePos inside a spinlock, and it will only ever increment. As SpinLockAcquire() has to be a read barrier we can assume that every skewed read in LWLockWait() will be for lock protecting a newer insertingAt? 2) I am not particularly happy about the LWLockWait() LWLockWakeup() function names. They sound too much like a part of the normal lwlock implementation to me. But admittedly I don't have a great idea for a better naming scheme. Maybe LWLockWaitForVar(), LWLockWakeupVarWaiter()? 3) I am the wrong one to complain, I know, but the comments above struct WALInsertLock are pretty hard to read from th sentence structure. 4) WALInsertLockAcquire() needs to comment on acquiring/waking all but the last slot. Generally the trick of exclusive xlog insertion lock acquiration only really using the last lock could use a bit more docs. 5) WALInsertLockRelease() comments on the reset of insertingAt being optional, but I am not convinced that that's true anymore. If an exclusive acquiration isn't seen as 0 or INT64CONST(0xFFFFFFFFFFFFFFFF) by another backend we're in trouble, right? Absolutely not sure without thinking on it for longer than I can concentrate right now. 6) Pretty minor, but from a style POV it seems nicer to separate exclusive/nonexclusive out of WALInsertLockAcquire(). The cases don't share any code now. A patch contianing some trivial changes is attached... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 484b9c5..8a55c6b 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -1628,8 +1628,6 @@ WALInsertLockRelease(void) static void WALInsertLockWakeup(XLogRecPtr insertingAt) { - int i; - if (holdingAllLocks) { /* diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index f88bf76..2695128 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -873,6 +873,9 @@ LWLockWait(LWLock *l, uint64 *valptr, uint64 oldval, uint64 *newval) int extraWaits = 0; bool result = false; + /* can't be used with shared locks for now */ + Assert(lock->shared == 0); + /* * Quick test first to see if it the slot is free right now. * @@ -905,6 +908,8 @@ LWLockWait(LWLock *l, uint64 *valptr, uint64 oldval, uint64 *newval) SpinLockAcquire(&lock->mutex); #endif + Assert(lock->shared == 0); + /* Is the lock now free, and if not, does the value match? */ if (lock->exclusive == 0) { @@ -1022,6 +1027,7 @@ LWLockWakeup(LWLock *l, uint64 *valptr, uint64 val) SpinLockAcquire(&lock->mutex); /* we should hold the lock */ + LWLockHeldByMe(l); Assert(lock->exclusive == 1); /* Update the lock's value */
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers