Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

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