Re: group locking: incomplete patch, just for discussion - Mailing list pgsql-hackers

On 2014-10-31 21:25:02 -0400, Robert Haas wrote:
> On Fri, Oct 31, 2014 at 4:10 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> I don't think that's correct.  We only need to process local
> >> invalidation messages after CommandCounterIncrement(), which I
> >> anticipate prohibiting during parallel execution (short thought should
> >> convince you that anything else is completely nuts).
> >
> > It is more complex, even without CCI. As long as you're doing anything
> > inside a transaction that already has done DDL, you'll have to play
> > nasty tricks. Imagine the primary backend has done a BEGIN; CLUSTER
> > pg_class; and then then starts a child backend. Which will get the same
> > snapshot, combocids, yada yada. But it *also* will have preexisting
> > cache entries. Those you need to invalidate before it can do anything
> > correct.
> 
> Where will those preexisting cache entries come from, exactly?  The
> postmaster is forking the parallel worker, not the user backend.

Several things:
1) The relcache init files load a fair bit
2) There's cache entries made just during startup while we look up user  information and such
3) There's lots of places you can hook into where it's perfectly legal  and expected that you access system caches. I
don'tthink you can  reasonably define those away.
 
4) I'm pretty sure that one of the early next steps will be to reuse a  bgworker for multiple tasks. So there'll
definitelybe preexisting  cache entries.
 

I'm pretty sure that it's impossible to define the problem away. We
*might* be able to say that we'll just do a InvalidateSystemCaches() at
start of the parallelized task.

> >> > I'm sorry to be a bit pessimistic here. But my intuition says that
> >> > starting to do group locking opens a can of worms that'll take us a long
> >> > time to close again. Maybe I'm just imagining complexity that won't
> >> > actually be there. But I have a hard time believing that.
> >>
> >> What's the distinction between "teach the deadlock detector to catch
> >> these kind of cases" and "group locking"?  Because I think those are
> >> at least 90% the same thing.
> >
> > I understand under 'group locking' that a primary/second backends can
> > coown a lock that normally is self-exclusive. That's a fair bit more
> > than adding an edge to the deadlock graph between primary/secondary
> > backends to make the deadlock detector recognize problems.
> 
> I guess.  It seems like a pretty minor extension to me.  Getting the
> deadlock detector to do the right thing is the hard part.

I doubt it. There's a whole bunch of problems that just exist by virtue
of having a group leader. What if the leader dies but the worker backend
isn't in a section of code that does a CHECK_FOR_INTERRUPTS()?

I also still have serious doubts about allowing self exclusive locks to be
shared between backends. That's going to cause pain. And that's pretty
much the only reason why you need to share entries.

> > What I have serious doubts about is 'coowning' locks. Especially if two
> > backends normally wouldn't be able to both get that lock.
> 
> Perhaps we should discuss that more.  To me it seems pretty obvious
> that's both safe and important.

I completely fail to see why you'd generally think it's safe for two
backends to hold the same self conflicting lock. Yes, within carefully
restricted sections of code that might be doable. But I don't think you
can fully enforce that. People *will* mark their own functions at
parallel safe. And do stupid stuff. That shouldn't cause
segfaults/exploits/low level data corruption/.. as long as it's done
from !C functions.

The aforementioned issue of how to deal with the leader dying while the
other backend still progresses also is nontrivial in my opinion.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services