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

On Mon, 9 Mar 2020 at 14:16, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Mar 8, 2020 at 7:58 AM Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
>>
>> On Mon, 24 Feb 2020 at 19:08, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >
>> > On Thu, Feb 20, 2020 at 8:06 AM Andres Freund <andres(at)anarazel(dot)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.

I think that could be a problem if we change the group locking so that
it doesn't consider page lock type.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services