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

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>
Cc: Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
Date: 2020-03-13 02:59:27
Message-ID: CAA4eK1LAr-_1JURScy1DQuqZUrWxP0V=89jieQYJJLzbdfXCXg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2020-03-13 03:07:22 Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
Previous Message Andy Fan 2020-03-13 01:47:10 Re: [PATCH] Erase the distinctClause if the result is unique by definition