Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

Hi,

I see you've committed this, cool. Sorry for not getting back to the
topic earlier..

On 2014-03-13 22:44:03 +0200, Heikki Linnakangas wrote:
> 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 don't find that argument convincing. After all it's only correct
because the API user does things in a particular way. So there should be
comment at the callsite to make sure that's not changed.

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

Yes, it is.

The committed version doesn't compile with LWLOCK_STATS...

Greetings,

Andres Freund

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