Re: Memory ordering issue in LWLockRelease, WakeupWaiters, WALInsertSlotRelease

On 02/10/2014 08:33 PM, Heikki Linnakangas wrote:
> On 02/10/2014 08:03 PM, Tom Lane wrote:
>> Heikki Linnakangas <hlinnakangas(at)vmware(dot)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.

This is what I came up with. I like it, I didn't have to contort lwlocks
as much as I feared. I added one field to LWLock structure, which is
used to store the position of how far a WAL inserter has progressed. The
LWLock code calls it just "value", without caring what's stored in it,
and it's used by new functions LWLockWait and LWLockWakeup to implement
the behavior the WAL insertion slots have, to wake up other processes
waiting for the slot without releasing it.

This passes regression tests, but I'll have to re-run the performance
tests with this. One worry is that if the padded size of the LWLock
struct is smaller than cache line, neighboring WAL insertion locks will
compete for the cache line. Another worry is that since I added a field
to LWLock struct, it might now take 64 bytes on platforms where it used
to be 32 bytes before. That wastes some memory.

- Heikki