On Fri, Mar 13, 2020 at 11:08 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 12, 2020 at 3:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Mar 11, 2020 at 2:36 PM Amit Kapila <amit.kapila16@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?
ok
> 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.
Another thing is
> to see if it makes sense to have a macro like LOCALLOCK_LOCKMETHOD to
> get the lock tag.
ok
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com