On Wed, Oct 22, 2014 at 7:12 PM, Andres Freund <and...@2ndquadrant.com>
wrote:
>
> On 2014-10-22 13:32:07 +0530, Amit Kapila wrote:
> > 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.
>
> After I've thought more about it, it's is actually required. This
> essentially *is* a retry.
Won't it needs to be set before retry? Whats the use of setting it
when we have got the lock and we are not going to retry.
> Someobdy woke us up, which is where releaseOK is supposed to be set.
I think that is true only in case when we are again going to retry or
atleast that seems to be the mechanism used currently in
LWLockAcquireCommon.
>
> > >> 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.
>
> I think you're placing unneccessarily high consistency constraints on a
> debugging feature here.
This was just a very minor suggestion to keep code consistent,
which if you want to ignore is okay. I understand that having
or not having code consistent for this doesn't matter.
> > 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?
>
> No. PGPROCs aren't deallocated or anything. And it's a debugging only
> variable.
Thats right, the problem I was thinking is of wrong information.
Ex. if process holding Exclusive locker has exited and then
lot of other processes took shared locks and one new Exclusive
locker waits on getting the lock, at that moment during debugging
we can get wrong information about lock owner.
However I think you are mainly worried about situtions when many
backends are waiting for Exclusive locker which is probably the
most common scenario.
> > 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))
> > ..
> > }
>
> What's the concern you have? Full memory barriers (the atomic
> nwaiters--) pair with read memory barriers.
IIUC, then pairing with nwaiters in LWLockAcquireCommon() ensures
that releaseOK is set before again attemting for lock as atomic
operation provides the necessary barrier. The point I am not
getting is what kind of guarantee pg_read_barrier() provides us
or why is it required?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
