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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
