Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

On 02/10/2014 08:03 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 02/10/2014 06:41 PM, Andres Freund wrote:
>>> Well, it's not actually using any lwlock.c code, it's a special case
>>> locking logic, just reusing the datastructures. That said, I am not
>>> particularly happy about the amount of code it's duplicating from
>>> lwlock.c. Pretty much all of WALInsertSlotReleaseOne and most of
>>> WALInsertSlotAcquireOne() is a copied.
>
>> I'm not too happy with the amount of copy-paste myself, but there was
>> enough difference to regular lwlocks that I didn't want to bother all
>> lwlocks with the xlog-specific stuff either. The WAL insert slots do
>> share the LWLock-related PGPROC fields though, and semaphore. I'm all
>> ears if you have ideas on that..
>
> I agree that if the behavior is considerably different, we don't really
> want to try to make LWLockAcquire/Release cater to both this and their
> standard behavior.  But why not add some additional functions in lwlock.c
> that do what xlog wants?  If we're going to have mostly-copy-pasted logic,
> it'd at least be better if it was in the same file, and not somewhere
> that's not even in the same major subtree.

Ok, I'll try to refactor it that way, so that we can see if it looks better.

> Also, the reason that LWLock isn't an abstract struct is because we wanted
> to be able to embed it in other structs; *not* as license for other
> modules to fool with its contents.  If we were working in C++ we'd
> certainly have made all its fields private.

Um, xlog.c is doing no such thing. The insertion slots use a struct of 
their own, which is also copy-pasted from LWLock (with one additional 
field).

- Heikki