Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

On 2014-02-14 13:28:47 +0100, Florian Pflug wrote:
> > I don't think that can actually happen because the head of the wait list
> > isn't the lock holder's lwWaitLink, but LWLock->head. I thought the same
> > for a while...
>
> Hm, true, but does that protect us under all circumstances? If another
> backend grabs the lock before B gets a chance to do so, B will become the
> wait queue's head, and anyone who enqueues behind B will do so by updating
> B's lwWaitLink. That is then undone by the delayed reset of B's lwWaitLink
> by the original lock holder.
>
> The specific sequence I have in mind is:
>
> A: Take lock
> B: Enqueue
> A: Release lock, set B's lwWaiting to false
> D: Acquire lock
> B: Enqueue after spurious wakeup
> (lock->head := B)
> C: Enqueue behind B
> (B->lwWaitLink := C, lock->tail := C)
> A: Set B's wWaitLink back to NULL, thereby corrupting the queue
> (B->lwWaitLink := NULL)
> D: Release lock, dequeue and wakeup B
> (lock->head := B->lwWaitLink, i.e. lock->head := NULL)
> B: Take and release lock, queue appears empty!
> C blocks indefinitely.

That's assuming either reordering by the compiler or an out-of-order
store architecture, right?

> >> I wonder whether LWLockRelease really needs to update lwWaitLink at all.
> >> We take the backends we awake off the queue by updating the queue's head and
> >> tail, so the contents of lwWaitLink should only matter once the backend is
> >> re-inserted into some wait queue. But when doing that, we reset lwWaitLink
> >> back to NULL anway.
> >
> > I don't like that, this stuff is hard to debug already, having stale
> > pointers around doesn't help. I think we should just add the barrier in
> > the releases with barrier.h and locally use a volatile var in the
> > branches before that.
>
> I like the idea of updating lwWaiting and lwWaitLink while still holding the
> spinlock better. The costs are probably negligible, and it'd make the code much
> easier to reason about.

I agree we should do that, but imo not in the backbranches. Anything
more than than the minimal fix in that code should be avoided in the
stable branches, this stuff is friggin performance sensitive, and the
spinlock already is a *major* contention point in many workloads.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services