Re: group locking: incomplete patch, just for discussion

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.

>> > 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.

> 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 wonder if we couldn't implement a 'halfway' by allowing parallel
>> > workers to jump the lockqueue if the parent process already has the
>> > lock. That'd only work for nonconflicting locks, but I think that's
>> > actually good.
>>
>> The patch takes precisely that approach; that part of the logic is
>> already implemented.
>
> Well, my point is that if the solution is just to jump the queue, there
> really isn't any data structure changes needed. Secondary acquirers just
> need to check whether a lock is already owned by the primary and then
> acquire the lock in the absolutely normal way - with a completely
> separate entry. Only that they ignored the queue.

Well, they need to be able to identify who is in their group.  You
could possibly do that without any changes at all to the lock manager
data structures, but it seems a bit complex.  I took the approach of
storing the group leader's PGPROC * in each PROCLOCK, which makes it
trivial (and is enough information for the deadlock detector to work
correctly, too).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company