Re: Wait free LW_SHARED acquisition - v0.2

On Tue, Oct 21, 2014 at 7:56 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> On Wed, Oct 8, 2014 at 6:17 PM, Andres Freund <andres(at)2ndquadrant(dot)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