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

On Mon, Mar 9, 2020 at 2:09 PM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Mon, 9 Mar 2020 at 15:50, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 9, 2020 at 11:38 AM Masahiko Sawada <
> masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> >>
> >> 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:
> >> >> >
> >> >> > 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.
> >
> >
> > I might be missing something, but won't that be a problem only when if
> there is a case where we acquire page lock after acquiring a relation
> extension lock?
>
> Yes, you're right.
>
> Well I meant that the reason why we need to make Assert should cover
> page locks case is the same as the reason for extension lock type
> case. If we change the group locking so that it doesn't consider
> extension lock and change deadlock so that it doesn't make a wait edge
> for it, we need to ensure that the same backend doesn't acquire
> heavy-weight lock after holding relation extension lock. These are
> already done in the current patch. Similarly, if we did the similar
> change for page lock in the group locking and deadlock , we need to
> ensure the same things for page lock.

Agreed.

> But ISTM it doesn't necessarily
> need to support page lock for now because currently we use it only for
> cleanup pending list of gin index.
>
>
I agree, but I think it is better to have a patch for the same even if we
want to review/commit that separately. That will help us to look at how
the complete solution looks.

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