On Mon, Mar 9, 2020 at 2:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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.
Please find the updated patch (summary of the changes) - Instead of searching the lock hash table for assert, it maintains a counter. - Also, handled the case where we can acquire the relation extension lock while holding the relation extension lock on the same relation. - Handled the error case. In addition to that prepared a WIP patch for handling the PageLock. First, I thought that we can use the same counter for the PageLock and the RelationExtensionLock because in assert we just need to check whether we are trying to acquire any other heavyweight lock while holding any of these locks. But, the exceptional case where we allowed to acquire a relation extension lock while holding any of these locks is a bit different. Because, if we are holding a relation extension lock then we allowed to acquire the relation extension lock on the same relation but it can not be any other relation otherwise it can create a cycle. But, the same is not true with the PageLock, i.e. while holding the PageLock you can acquire the relation extension lock on any relation and that will be safe because the relation extension lock guarantee that, it will never create the cycle. However, I agree that we don't have any such cases where we want to acquire a relation extension lock on the different relations while holding the PageLock. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
v3-0001-Add-assert-to-check-that-we-should-not-acquire-an.patch
Description: Binary data
v3-0003-Conflict-Extension-Page-lock-in-group-member.patch
Description: Binary data
v3-0002-WIP-Extend-the-patch-for-handling-PageLock.patch
Description: Binary data
