On Fri, Mar 13, 2020 at 11:16 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Fri, Mar 13, 2020 at 11:08 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Thu, Mar 12, 2020 at 3:04 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > >
> > > On Wed, Mar 11, 2020 at 2:36 PM Amit Kapila <amit.kapil...@gmail.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?
>
> okDone > > > 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
v6-0001-Add-assert-check-for-relation-extension-lock.patch
Description: Binary data
v6-0004-Page-lock-to-conflict-among-parallel-group-member.patch
Description: Binary data
v6-0002-Extend-the-assert-for-the-page-lock.patch
Description: Binary data
v6-0003-Relation-extension-lock-to-conflict-among-paralle.patch
Description: Binary data
