Horton

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

Dilip Kumar <dilipbalaut@gmail.com>

View thread
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Mahendra Singh Thalor <mahi6run@gmail.com>, Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Masahiko Sawada <masahiko.sawada@2ndquadrant.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Michael Paquier <michael@paquier.xyz>, Mithun Cy <mithun.cy@enterprisedb.com>, Thomas Munro <thomas.munro@enterprisedb.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-03-13T05:46:00Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow page lock to conflict among parallel group members.

  2. Allow relation extension lock to conflict among parallel group members.

  3. Add assert to ensure that page locks don't participate in deadlock cycle.

  4. Assert that we don't acquire a heavyweight lock on another object after

  5. Fix unsafe usage of strerror(errno) within ereport().

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