Re: group locking: incomplete patch, just for discussion

On Thu, Nov 20, 2014 at 09:15:51PM +0100, Andres Freund wrote:
> On 2014-11-20 13:57:25 -0500, Robert Haas wrote:
> > On Thu, Nov 20, 2014 at 12:19 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > If the
> > deadlock detector would kill the processes anyway, it doesn't matter,
> > because CheckTableNotInUse() should do it first, so that we get a
> > better error and without waiting for deadlock_timeout. So any
> > scenario that's predicated on the assumption that CheckTableNotInUse()
> > will succeed in a parallel context is 100% unconvincing to me as an
> > argument for anything.
>
> Meh, there's plenty cases where CheckTableNotInUse() isn't used where we
> still rely on locks actually working. And it's not just postgres code,
> this *will* break user code.

Today, holding a relation AccessExclusiveLock ensures no other process is
using the table. That is one of two prerequisites for making an alteration
that threatens concurrent users of the table. The other prerequisite is
verifying that no other subsystem of the current process is using the table,
hence CheckTableNotInUse(). Robert's designs do soften AccessExclusiveLock
for relations; holding it will not preclude use of the table in workers of the
same parallel group. Pessimizing CheckTableNotInUse() compensates. In every
scenario where AccessExclusiveLock is softer than before, CheckTableNotInUse()
will fail unconditionally. Code that, today, performs the correct checks
before making a threatening table alteration will remain correct.

ANALYZE and lazy VACUUM are the only core operations that take self-exclusive
relation locks and skip CheckTableNotInUse(). PreventTransactionChain() will
block naive concurrent VACUUM. (If that weren't the case, VACUUM would need
CheckTableNotInUse() even without parallelism in the picture. I wouldn't
oppose adding CheckTableNotInUse() to VACUUM as defense in depth.) ANALYZE is
special. It's compatible with most concurrent use of the table, so
CheckTableNotInUse() is unnecessary. Blocking ANALYZE during parallelism,
even if we someday allow other database writes, is a wart I can live with.

Given a choice between no parallelism and parallelism when the initiating
transaction holds no self-exclusive locks, I'd pick the latter. But you have
overstated the significance of holding a lock and the degree to which Robert's
proposals change the same.

> Your argument essentially is that we don't actually need to lock objects
> if the other side only reads. Yes, you add a condition or two ontop
> (e.g. CheckTableNotInUse() fails), but that's not that much. If we want
> to do this we really need to forbid *any* modification to catalog/user
> tables while parallel operations are ongoing. In the primary and worker
> backends. I think that's going to end up being much more
> problematic/restrictive than not allowing non-cooperative paralellism
> after >= ShareUpdateExclusive. If we ever want to parallelize writing
> operations we'll regret this quite a bit.

https://wiki.postgresql.org/wiki/Parallel_Sort already contemplates blocking
every kind of database write, due to challenges around combo CIDs (affects
UPDATE and DELETE) and invalidation messages (mostly affects DDL). I doubt
we'll ever care to allow a worker to start an independent DDL operation. We
might want to allow INSERT, UPDATE, and DELETE. I don't see that allowing
multiple workers to hold AccessExclusiveLock will make that more difficult.
Difficulties around XactLockTableWait() are more noteworthy, and they stand
regardless of what we do with heavyweight locking.

> The 'lets grant all the current locks at start of parallel query'
> approach seems quite a bit safer than always doing that during parallel
> query, don't get me wrong.

Agreed.

> Btw, what are your thoughts about SERIALIZABLE and parallel query?
> Afaics that'd be pretty hard to support?

It didn't look obviously awful, because most of the relevant data structures
live in shared memory. We already have the concept of one backend adjusting
the predicate locks pertaining to another backend's transaction. (That's not
to say the number of SERIALIZABLE-specific changes will be zero, either.)

Thanks,
nm