On Tue, Oct 21, 2014 at 7:56 PM, Amit Kapila <amit.kapil...@gmail.com>
wrote:
> On Wed, Oct 8, 2014 at 6:17 PM, Andres Freund <and...@2ndquadrant.com>
wrote:
> >
> > On 2014-06-25 19:06:32 +0530, Amit Kapila wrote:
Today, I have verified all previous comments raised by
me and looked at new code and below are my findings:
>>
>> 4.
>> LWLockAcquireCommon()
>> {
>> ..
>> if (!LWLockDequeueSelf(l))
>> {
>> for (;;)
>> {
>> PGSemaphoreLock(&proc->sem, false);
>> if (!proc->lwWaiting)
>> break;
>> extraWaits++;
>> }
>> lock->releaseOK = true;
>> ..
>> }
>>
>> Setting releaseOK in above context might not be required because if the
>> control comes in this part of code, it will not retry to acquire another
>> time.
> Hm. You're probably right.
You have agreed to fix this comment, but it seems you have forgot
to change it.
>> 11.
>> LWLockRelease()
>> {
>> ..
>> PRINT_LWDEBUG("LWLockRelease", lock, mode);
>> }
>>
>> Shouldn't this be in begining of LWLockRelease function rather than
>> after processing held_lwlocks array?
> Ok.
You have agreed to fix this comment, but it seems you have forgot
to change it.
Below comment doesn't seem to be adressed?
> LWLockAcquireOrWait()
> {
> ..
> LOG_LWDEBUG("LWLockAcquireOrWait", lockid, mode, "succeeded");
> ..
> }
> a. such a log is not there in any other LWLock.. variants,
> if we want to introduce it, then shouldn't it be done at
> other places as well.
Below point is yet to be resolved.
> > 12.
> > #ifdef LWLOCK_DEBUG
> > lock->owner = MyProc;
> > #endif
> >
> > Shouldn't it be reset in LWLockRelease?
>
> That's actually intentional. It's quite useful to know the last owner
> when debugging lwlock code.
Won't it cause any problem if the last owner process exits?
Can you explain how pg_read_barrier() in below code makes this
access safe?
LWLockWakeup()
{
..
+ pg_read_barrier(); /* pairs with nwaiters-- */
+ if (!BOOL_ACCESS_ONCE(lock->releaseOK))
..
}
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
