Re: group locking: incomplete patch, just for discussion

On Fri, Oct 31, 2014 at 3:32 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > So just refusing parallelism as soon as anything has taken an access
>> > exclusive lock doesn't sound too bad to me.
>>
>> That restriction seems onerous to me; for example, it would prevent a
>> parallel sort for CLUSTER or a parallel index rebuild for VACUUM FULL.
>> Those seems like worthy targets for parallelism, and maybe even early
>> targets, since I expect a lot of the real complexity here to be in the
>> query planner, and you can avoid most of that complexity when planning
>> DDL.
>
> Ugh. I think that's aiming too high. You'll suddenly need to share cche
> invalidations between the backends. I think we should start by requiring
> that there's no DDL done *at all* in the transaction that starts the
> parallel activity. For CREATE INDEX PARALLEL that can be done by reusing
> the logic we have for CONCURRENTLY, thereby moving the parallelized part
> into a transaction that hasn't done any DDL yet.

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).  So I think
there's no real problem with invalidation messages being generated
during parallel execution.  I wouldn't anticipate supporting that in
V1, because they'd have to be propagated back to the user backend
prior to commit, but in the longer term it seems pretty feasible.

>> I also think it's unnecessary.  It's wrong to think of two parallel
>> backends that both want AccessExclusiveLock on a given target relation
>> as conflicting with each other; if the process were not running in
>> parallel, those lock requests would both have come from the same
>> process and both requests would have been granted.
>
> I don't think that's a realistic view. There's lots of backend private
> state around. If we try to make all that coherent between backends we'll
> fail.

I agree that there's a lot of backend private state, and that's the
principle problem with making this work at all.  The challenge here is
to figure out which parts of that backend-private state we absolutely
must make coherent between backends to enable parallelism to have a
useful, non-trivial scope, and which parts we can punt on.  I have
ideas about that - which are mostly documented at
https://wiki.postgresql.org/wiki/Parallel_Sort - but I'm trying to
keep an open mind on the details.

>> Parallelism is
>> generally not about making different things happen; it's about
>> spreading the stuff that would happen anyway across multiple backends,
>> and things that would succeed if done in a single backend shouldn't
>> fail when spread across 2 or more without some compelling
>> justification.  The cases where it's actually unsafe for a table to be
>> accessed even by some other code within the same backend are handled
>> not by the lock manager, but by CheckTableNotInUse().  That call
>> should probably fail categorically if issued while parallelism is
>> active.
>
> Which will e.g. prohibit CLUSTER and VACUUM FULL.

I don't think so.  I think it's safe to get to the point where we
check that the table is not in use, then do a parallel scan and sort,
then collect the results and write them out.  But I would not try to
make it safe, for example, to be scanning a table and have some
user-defined function invoked along the way to go try to do anything
that involves CheckTableNotInUse().  Many of those would fall over
anyway because of PreventTransactionChain(), but I've got no problem
nailing that hatch shut twice.  And conversely, I would expect we'd
disallow the sort operators invoked by CLUSTER from reaching any code
guarded by CheckTableNotInUse().

>> Suppose somebody fires off a parallel sort on a text column, or a
>> parallel sequential scan involving a qual of the form textcol = 'zog'.
>> We launch a bunch of workers to do comparisons; they do lookups
>> against pg_collation.  After some but not all of them have loaded the
>> collation information they need into their local cache, the DBA types
>> "cluster pg_collate".  It queues up for an AccessExclusiveLock.  The
>> remaining parallel workers join the wait queue waiting for their
>> AccessShareLock. The system is now deadlocked; the only solution is to
>> move the parallel workers ahead of the AccessExclusiveLock request,
>> but the deadlock detector won't do that unless it knows about the
>> relationship among the parallel workers.
>
> I think you would need to make the case more complex - we only hold
> locks on system object for a very short time, and mostly not in a nested
> fashion.

Hmm.  You're right.  If the parallel backends release their locks
quickly, the CLUSTER could run and then the parallel job could finish.
But it sounds as if you get the idea.

> But generally I think it's absolutely perfectly alright to fail in such
> case. We need to fail reliably, but *none* of this is a new hazard. You
> can have pretty similar issues today, without any parallelism.

I think it's absolutely fine for it to *fail*.  I think it's not OK
for it to *hang* without triggering the deadlock detector.

> Well, that's why I think we should teach the deadlock detector to catch
> these kind of cases. That doesn't sound particularly hard.

I could not agree more!  In fact, that's the whole point of the patch.

> 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 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.  I'd like to make it a bit more robust to handle
cases where the deadlock detector gets involved, as I currently
believe that (a) can be done without unmanageable complexity, (b) will
make programming in parallel mode much simpler, and (c) will
significantly decrease the chances of an angry user or
fellow-committer tracking me down and beating me to death with a
keen-edged bug report.

> That's in the current prototype you sent or something newer you have
> privately?

The former.

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