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?
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. Another thing is
to see if it makes sense to have a macro like LOCALLOCK_LOCKMETHOD to
get the lock tag.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com