Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

On Fri, Mar 13, 2020 at 11:16 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Mar 13, 2020 at 11:08 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 12, 2020 at 3:04 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Mar 11, 2020 at 2:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > >
> > > > If we have no other choice, then I see a few downsides of adding a
> > > > special check in the LockRelease() call:
> > > >
> > > > 1. Instead of resetting/decrement the variable from specific APIs like
> > > > UnlockRelationForExtension or UnlockPage, we need to have it in
> > > > LockRelease. It will also look odd, if set variable in
> > > > LockRelationForExtension, but don't reset in the
> > > > UnlockRelationForExtension variant. Now, maybe we can allow to reset
> > > > it at both places if it is a flag, but not if it is a counter
> > > > variable.
> > > >
> > > > 2. One can argue that adding extra instructions in a generic path
> > > > (like LockRelease) is not a good idea, especially if those are for an
> > > > Assert. I understand this won't add anything which we can measure by
> > > > standard benchmarks.
> > >
> > > I have just written a WIP patch for relation extension lock where
> > > instead of incrementing and decrementing the counter in
> > > LockRelationForExtension and UnlockRelationForExtension respectively.
> > > We can just set and reset the flag in LockAcquireExtended and
> > > LockRelease. So this patch appears simple to me as we are not
> > > involving the transaction APIs to set and reset the flag. However, we
> > > need to add an extra check as you have already mentioned. I think we
> > > could measure the performance and see whether it has any impact or
> > > not?
> > >
> >
> > LockAcquireExtended()
> > {
> > ..
> > + if (locktag->locktag_type == LOCKTAG_RELATION_EXTEND)
> > + IsRelationExtensionLockHeld = true;
> > ..
> > }
> >
> > Can we move this check inside a function (CheckAndSetLockHeld or
> > something like that) as we need to add a similar thing for page lock?
>
> ok

Done

>
> > Also, how about moving the set and reset of these flags to
> > GrantLockLocal and RemoveLocalLock as that will further reduce the
> > number of places where we need to add such a check.
>
> Make sense to me.

Done
>

> Another thing is
> > to see if it makes sense to have a macro like LOCALLOCK_LOCKMETHOD to
> > get the lock tag.
>
> ok

Done

Apart from that, I have also extended the solution for the page lock.
And, I have also broken down the 3rd patch in two parts for relation
extension and for the page lock.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com