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

On Fri, Mar 13, 2020 at 8:29 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Mar 12, 2020 at 7:50 PM Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 12, 2020 at 5:28 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Mar 12, 2020 at 11:15 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > I have fixed this in the attached patch set.
> > > >
> > >
> > > I have modified your
> > > v4-0003-Conflict-Extension-Page-lock-in-group-member patch. The
> > > modifications are (a) Change src/backend/storage/lmgr/README to
> > > reflect new behaviour, (b) Introduce a new macro LOCK_LOCKTAG which
> > > slightly simplifies the code, (c) moved the deadlock.c check a few
> > > lines up and (d) changed a few comments.
> > >
> > > It might be better if we can move the checks related to extension and
> > > page lock in a separate API or macro. What do you think?
> > >
> > I think moving them inside a macro is a good idea. Also, I think we
> > should move all the Assert related code inside some debugging macro
> > similar to this:
> > #ifdef LOCK_DEBUG
> > ....
> > #endif
> >
>
> If we move it under some macro, then those Asserts will be only
> enabled when that macro is defined. I think we want there Asserts to
> be enabled always in assert enabled build, these will be like any
> other Asserts in the code. What is the advantage of doing those under
> macro?
>
> > + /*
> > + * The relation extension or page lock can never participate in actual
> > + * deadlock cycle. See Asserts in LockAcquireExtended. So, there is
> > + * no advantage in checking wait edges from it.
> > + */
> > + if ((LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND) ||
> > + (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE))
> > + return false;
> > +
> > Since this is true, we can also avoid these kind of locks in
> > ExpandConstraints, right?
> >
>
> Yes, I had also thought about it but left it to avoid sprinkling such
> checks at more places than absolutely required.
>
> > It'll certainly reduce some complexity in
> > topological sort.
> >
>
> I think you mean to say TopoSort will have to look at fewer members in
> the wait queue, otherwise, there is nothing from the perspective of
> code which we can remove/change there. I think there will be hardly
> any chance that such locks will participate here because we take those
> for some work and release them (basically, they are unlike other
> heavyweight locks which can be released at the end). Having said
> that, I am not against putting those checks at the place you are
> suggesting, it is just that I thought that it won't be of much use.

I am not sure I understand this part. Because topological sort will
work on the soft edges we have created when we found the cycle, but
for relation extension/page lock we are completely ignoring hard/soft
edge then it will never participate in topo sort as well. Am I
missing something?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com