Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

On 03/12/2014 09:29 PM, Andres Freund wrote:
> 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().

There's a comment inside LWLockWait(). I think that's the right place
for it; it's LWLockWait() that's cheating by not acquiring the spinlock
before reading lock->exclusive.

> 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?

Right. If the quick test in the beginning of LWLockWait() returns
'true', even though the lock was *just* acquired by a different backend,
it nevertheless must've been free some time after we read CurrBytePos in
WaitXLogInsertionsToFinish(), and that's good enough.

> 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()?

Works for me.

> 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.

Hmm, ok. I reworded that, I hope it's more clear now.

> 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.

Added.

> 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.

Hmm, right, it isn't optional when resetting the 0xFFFFFFFFFFFFFFFF
value back to zero.

Now that I look at it, even resetting it to zero in the normal,
non-exclusive case is more fiddly than it seems at first glance
(although still correct). We do this:

0. Acquire the WAL insertion lock, get insert position
1. Copy the WAL data to the shared buffer
2. Set insertingAt = 0
3. Release the lock.

However, nothing stops the compiler (or CPU on weak-memory-order
architectures) from rearranging the operations like this:

0. Acquire the WAL insertion lock, get insert position
1. Set insertingAt = 0
2. Copy the WAL data to the shared buffer
3. Release the lock.

Furthermore, setting the insertingAt value might be "torn" if a 64-bit
store is not atomic. That would be a problem, if a backend saw the torn
value, and incorrectly determined that it doesn't need to wait for it.
(If the compiler didn't reorder steps 1 and 2, that would be OK because
by the time we reset insertingAt, we have already copied the WAL data.)

We're saved by the fact that resetting insertingAt to 0 never moves the
value forwards, only backwards, even if someone sees a torn value. If we
used a some magic value other than 0 to mean "uninitialized", we would
have trouble. But that's way more fiddly than I'd like, so let's make
that more robust.

What we really ought to do is to initialize insertingAt inside
LWLockAcquire (or LWLockRelease), while we're holding the lwlock's
spinlock. The only reason I didn't do that was to avoid having another
copy of LWLockAcquire, with the 'var', but maybe that was penny-wise and
pound-foolish.


New patch version attached. I added a new variant of LWLockAcquire,
called LWLockAcquireWithVar, to reset the variable atomically when the
lock is acquired. LWLockAcquire and the new function now just call an
internal function that implements both, but I'm now slightly worried if
that might hurt performance of LWLockAcquire in general. The extra
indirection through the function call shouldn't add much overhead, but
LWLockAcquire is called so frequently that every cycle counts.

- Heikki