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

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.

> /*
> + * The relation extension or page lock conflict even between the group
> + * members.
> + */
> + if ((LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND) ||
> + (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE))
> + {
> + PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)",
> + proclock);
> + return true;
> + }
> This check includes the heavyweight locks that conflict even under
> same parallel group. It also has another property that they can never
> participate in deadlock cycles. And, the number of locks under this
> category is likely to increase in future with new parallel features.
> Hence, it could be used in multiple places. Should we move the
> condition inside a macro and just call it from here?
>

Right, this is what I have suggested upthread. Do you have any
suggestions for naming such a macro or function? I could think of
something like LocksConflictAmongGroupMembers or
LocksNotParticipateInDeadlock. The first one suits more for its usage
in LockCheckConflicts and the second in the deadlock.c code. So none
of those sound perfect to me.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com