On Mon, 24 Feb 2020 at 15:39, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Feb 20, 2020 at 8:06 AM Andres Freund <and...@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). > b. Change lock.c so that group locking is not considered for these two > lock types. For ex. in LockCheckConflicts, along with the check (if > (proclock->groupLeader == MyProc && MyProc->lockGroupLeader == NULL)), > we also check lock->tag and call it a conflict for these two locks. > c. The deadlock detector can ignore checking these two types of locks > because point (a) ensures that those won't lead to deadlock. One idea > could be that FindLockCycleRecurseMember just ignores these two types > of locks by checking the lock tag.
Thanks Amit for summary. Based on above 3 points, here attaching 2 patches for review. 1. v01_0001-Conflict-EXTENTION-lock-in-group-member.patch (Patch by Dilip Kumar) Basically this patch is for point b and c. 2. v01_0002-Added-assert-to-verify-that-we-never-try-to-take-any.patch (Patch by me) This patch is for point a. After applying both the patches, make check-world is passing. We are testing both the patches and will post results. Thoughts? -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
v01_0001-Conflict-EXTENTION-lock-in-group-member.patch
Description: Binary data
v01_0002-Added-assert-to-verify-that-we-never-try-to-take-any.patch
Description: Binary data
