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: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.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-09T05:16:34Z
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 Sun, Mar 8, 2020 at 7:58 AM Masahiko Sawada <
masahiko.sawada@2ndquadrant.com> wrote:

> On Mon, 24 Feb 2020 at 19:08, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 8:06 AM Andres Freund <andres@anarazel.de>
> wrote:
> > >
> > > Hi,
> > >
> > > On 2020-02-19 11:12:18 +0530, Amit Kapila wrote:
> > > > I think till we know the real need for changing group locking, going
> > > > in the direction of what Tom suggested to use an array of LWLocks [1]
> > > > to address the problems in hand is a good idea.
> > >
> > > -many
> > >
> > > I think that building yet another locking subsystem is the entirely
> > > wrong idea - especially when there's imo no convincing architectural
> > > reasons to do so.
> > >
> >
> > Hmm, AFAIU, it will be done by having an array of LWLocks which we do
> > at other places as well (like BufferIO locks).  I am not sure if we
> > can call it as new locking subsystem, but if we decide to continue
> > using lock.c and change group locking then I think we can do that as
> > well, see my comments below regarding that.
> >
> > >
> > > > It is not very clear to me that are we thinking to give up on Tom's
> > > > idea [1] and change group locking even though it is not clear or at
> > > > least nobody has proposed an idea/patch which requires that?  Or are
> > > > we thinking that we can do what Tom suggested for relation extension
> > > > lock and also plan to change group locking for future parallel
> > > > operations that might require it?
> > >
> > > What I'm advocating is that extension locks should continue to go
> > > through lock.c. And yes, that requires some changes to group locking,
> > > but I still don't see why they'd be complicated.
> > >
> >
> > Fair position, as per initial analysis, I think if we do below three
> > things, it should work out without changing to a new way of locking
> > for relation extension or page type locks.
> > a. As per the discussion above, ensure in code we will never try to
> > acquire another heavy-weight lock after acquiring relation extension
> > or page type locks (probably by having Asserts in code or maybe some
> > other way).
>
> The current patch
> (v02_0001-Added-assert-to-verify-that-we-never-try-to-take-any.patch)
> doesn't check that acquiring a heavy-weight lock after page type lock,
> is that right?


No, it should do that.


> There is the path doing that: ginInsertCleanup() holds
> a page lock and insert the pending list items, which might hold a
> relation extension lock.
>

Right, I could also see that, but do you see any problem with that?  I
agree that Assert should cover this case, but I don't see any fundamental
problem with that.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com