Re: [HACKERS] Moving relation extension locks out of heavyweightlock manager - Mailing list pgsql-hackers

Hi,

On 2020-02-14 12:08:45 -0500, Robert Haas wrote:
> On Fri, Feb 14, 2020 at 11:40 AM Andres Freund <andres@anarazel.de> wrote:
> > IMO the right thing here is to extend lock.c so we can better represent
> > whether certain types of lockmethods (& levels ?) are [not] to be
> > shared.
> 
> The part that I find awkward about that is the whole thing with the
> deadlock detector. The deadlock detection code is old, crufty,
> complex, and very difficult to test (or at least I have found it so).
> A bug that I introduced when inventing group locking took like 5 years
> for somebody to find.

Oh, I agree, lock.c and surrounding code is pretty crufty. Doubtful that
just building up a largely parallel piece of infrastructure next to it
is a good answer though.


> One way of looking at the requirement that we have here is that
> certain kinds of locks need to be exempted from group locking.
> Basically, these are because they are a lower-level concept: a lock on
> a relation is more of a "logical" concept, and you hold the lock until
> eoxact, whereas a lock on an extend the relation is more of a
> "physical" concept, and you give it up as soon as you are done. Page
> locks are like relation extension locks in this regard. Unlike locks
> on SQL-level objects, these should not be shared between members of a
> lock group.
> 
> Now, if it weren't for the deadlock detector, that would be easy
> enough. But figuring out what to do with the deadlock detector seems
> really painful to me. I wonder if there's some way we can make an end
> run around that problem. For instance, if we could make (and enforce)
> a coding rule that you cannot acquire a heavyweight lock while holding
> a relation extension or page lock, then maybe we could somehow teach
> the deadlock detector to just ignore those kinds of locks, and teach
> the lock acquisition machinery that they conflict between lock group
> members.

Yea, that seems possible.  I'm not really sure it's needed however? As
long as you're not teaching the locking mechanism new tricks that
influence the wait graph, why would the deadlock detector care? That's
quite different from the group locking case, where you explicitly needed
to teach it something fairly fundamental.

It might still be a good idea independently to add the rule & enforce
that acquire heavyweight locks while holding certain classes of locks is
not allowed.


> On the other hand, I think you might also be understating the
> differences between these kinds of locks and other heavyweight locks.
> I suspect that the reason why we use lwlocks for buffers and
> heavyweight locks here is because there are a conceptually infinite
> number of relations, and lwlocks can't handle that.

Right. For me that's *the* fundamental service that lock.c delivers. And
it's the fundamental bit this thread so far largely has been focusing
on.


> The only mechanism we currently have that does handle that is the
> heavyweight lock mechanism, and from my point of view, somebody just
> beat it with a stick to make it fit this application. But the fact
> that it has been made to fit does not mean that it is really fit for
> purpose. We use 2 of 9 lock levels, we don't need deadlock detection,
> we need different behavior when group locking is in use, we release
> locks right away rather than at eoxact. I don't think it's crazy to
> think that those differences are significant enough to justify having
> a separate mechanism, even if the one that is currently on the table
> is not exactly what we want.

Isn't that mostly true to varying degrees for the majority of lock types
in lock.c? Sure, perhaps historically that's a misuse of lock.c, but
it's been pretty ingrained by now.  I just don't see where leaving out
any of these features is going to give us fundamental advantages
justifying a different locking infrastructure.

E.g. not needing to support "conceptually infinite" number of relations
IMO does provide a fundamental advantage - no need for a mapping.  I'm
not yet seeing anything equivalent for the extension vs. lock.c style
lock case.

Greetings,

Andres Freund