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

On Wed, 4 Mar 2020 at 12:03, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Mar 4, 2020 at 11:45 AM Mahendra Singh Thalor
> <mahi6run(at)gmail(dot)com> wrote:
> >
> > On Mon, 24 Feb 2020 at 15:39, 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).
> > > 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?
>
> +static void AssertAnyExtentionLockHeadByMe(void);
>
> +/*
> + * AssertAnyExtentionLockHeadByMe -- test whether any EXTENSION lock held by
> + * this backend. If any EXTENSION lock is hold by this backend, then assert
> + * will fail. To use this function, assert should be enabled.
> + */
> +void AssertAnyExtentionLockHeadByMe()
> +{
>
> Some minor observations on 0002.
> 1. static is missing in a function definition.
> 2. Function name should start in new line after function return type
> in function definition, as per pg guideline.
> +void AssertAnyExtentionLockHeadByMe()
> ->
> void
> AssertAnyExtentionLockHeadByMe()

Thanks Dilip for review.

I have fixed above 2 points in v2 patch set.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com