Re: Sync Rep v17

On Tue, Mar 1, 2011 at 3:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Tue, 2011-03-01 at 15:51 +0900, Fujii Masao wrote:
>> Thanks for update of the patch!
>>
>> On Tue, Mar 1, 2011 at 3:40 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> >> SyncRepRemoveFromQueue seems not to be as short-term as we can
>> >> use the spinlock. Instead, LW lock should be used there.
>>
>> You seem to have forgotten to fix the above-mentioned issue.
>
> Not forgotten.
>
>> A spinlock can be used only for very short-term operation like
>> read/write of some shared-variables. The operation on the queue
>> is not short, so should be protected by LWLock, I think.
>
> There's no need to sleep while holding locks and the operations are very
> short in most cases. The code around it isn't trivial, but that's no
> reason to use LWlocks.
>
> LWlocks are just spinlocks plus sem sleeps, so I don't see the need for
> that in the current code. Other views welcome.

An LWLock is a lot safer, in general, than a spinlock.  A spinlock
mustn't do anything that could emit an error or abort (among other
things).  I doubt that the performance cost of using an LWLock rather
than a spin lock here is enough to matter, and the spin lock seems
more likely to result in hard-to-find bugs.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company