On Mon, Mar 9, 2020 at 2:09 PM Masahiko Sawada < masahiko.saw...@2ndquadrant.com> wrote:
> On Mon, 9 Mar 2020 at 15:50, Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Mon, Mar 9, 2020 at 11:38 AM Masahiko Sawada < > masahiko.saw...@2ndquadrant.com> wrote: > >> > >> On Mon, 9 Mar 2020 at 14:16, Amit Kapila <amit.kapil...@gmail.com> > wrote: > >> > > >> > On Sun, Mar 8, 2020 at 7:58 AM Masahiko Sawada < > masahiko.saw...@2ndquadrant.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
