Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Dilip Kumar <dilipbalaut@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:38:08Z
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 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