Re: parallel mode and parallel contexts

On 2015-02-11 13:59:04 -0500, Robert Haas wrote:
> On Tue, Feb 10, 2015 at 8:45 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > The only reason I'd like it to be active is because that'd *prohibit*
> > doing the crazier stuff. There seems little reason to not da it under
> > the additional protection against crazy things that'd give us?
>
> Trying to load additional libraries once parallel mode is in progress
> can provide failures, because the load of the libraries causes the
> system to do GUC_ACTION_SET on the GUCs whose initialization was
> deferred, and that trips up the
> no-changing-things-while-in-parallel-mode checks.

Oh, that's a good point.

> I'm not sure if there's anything we can, or should, do about that.

Fine with me.

> > Well, ExportSnapshot()/Import has quite a bit more restrictions than
> > what you're doing... Most importantly it cannot import in transactions
> > unless using read committed isolation, forces xid assignment during
> > export, forces the old snapshot to stick around for the whole
> > transaction and only works on a primary. I'm not actually sure it makes
> > a relevant difference, but it's certainly worth calling attention to.
> >
> > The fuding I was wondering about certainly is unnecessary though -
> > GetSnapshotData() has already performed it...
> >
> > I'm not particularly awake right now and I think this needs a closer
> > look either by someone awake... I'm not fully convinced this is safe.
>
> I'm not 100% comfortable with it either, but I just spent some time
> looking at it and can't see what's wrong with it. Basically, we're
> trying to get the parallel worker into a state that matches the
> master's state after doing GetTransactionSnapshot() - namely,
> CurrentSnapshot should point to the same snapshot on the master, and
> FirstSnapshotSet should be true, plus the same additional processing
> that GetTransactionSnapshot() would have done if we're in a higher
> transaction isolation level. It's possible we don't need to mimic
> that state, but it seems like a good idea.

I plan to look at this soonish.

> Still, I wonder if we ought to be banning GetTransactionSnapshot()
> altogether. I'm not sure if there's ever a time when it's safe for a
> worker to call that.

Why?

> >> > Also, you don't seem to
> >> > prohibit popping the active snapshot (should that be prohibitted
> >> > maybe?) which might bump the initiator's xmin horizon.
> >>
> >> I think as long as our transaction snapshot is installed correctly our
> >> xmin horizon can't advance; am I missing something?
> >
> > Maybe I'm missing something, but why would that be the case in a read
> > committed session? ImportSnapshot() only ever calls
> > SetTransactionSnapshot it such a case (why does it contain code to cope
> > without it?), but your patch doesn't seem to guarantee that.
> >
> > But as don't actually transport XactIsoLevel anywhere (omission
> > probably?) that seems to mean that the SetTransactionSnapshot() won't do
> > much, even if the source transaction is repeatable read.
>
> Doesn't XactIsoLevel get set by assign_XactIsoLevel() when we restore
> the GUC state?

Yes, but afaics the transaction start will just overwrite it again:

static void
StartTransaction(void)
{
...
XactDeferrable = DefaultXactDeferrable;
XactIsoLevel = DefaultXactIsoLevel;
...

For a client issued BEGIN it works because utility.c does:
case TRANS_STMT_BEGIN:
case TRANS_STMT_START:
{
ListCell *lc;

BeginTransactionBlock();
foreach(lc, stmt->options)
{
DefElem *item = (DefElem *) lfirst(lc);

if (strcmp(item->defname, "transaction_isolation") == 0)
SetPGVariable("transaction_isolation",
list_make1(item->arg),
true);
else if (strcmp(item->defname, "transaction_read_only") == 0)
SetPGVariable("transaction_read_only",
list_make1(item->arg),
true);
else if (strcmp(item->defname, "transaction_deferrable") == 0)
SetPGVariable("transaction_deferrable",
list_make1(item->arg),
true);
}

Pretty, isn't it?

> > Your manual ones don't either, that's what made me
> > complain. E.g. pg_advisory_lock_int8 isn't called that on the SQL
> > level. Instead they use the C name + parens.
>
> On further reflection, I think I should probably just change all of
> those to use a general message about advisory locks, i.e.:
>
> ERROR: cannot use advisory locks during a parallel operation
>
> That sound good to you?

Perfectly fine with me.

> > Which is *not* a good example for the problem. Your primary reasoning
> > for needing something more than sharing the locks that we know the
> > individual query is going to need (which we acquire in the parse
> > analzye/planner/executor) is that we can't predict what some random code
> > does. Now, I still don't think that argument should hold too much sway
> > because we'll only be able to run carefully controlled code
> > *anyway*.
>
> I'll avoid repeating what I've said about this before, except to say
> that I still don't believe for a minute you can predict which locks
> you'll need.

I don't understand. Leaving AEL locks on catalog tables aside, pretty
much everything else is easily visible? We already do that for RTE
permission checks and such? There might be some holes, but those should
rather be fixed anyway. What's so hard about determining the locks
required for a query?

> > But *if* we take it as reasoning for doing more than granting
> > the locks we need for the individual query, we *still* need to cope with
> > exactly the variants of the above invisible deadlock. You can still can
> > call a function acquiring some form of lock on either side.
>
> That by itself is not a deadlock. If the worker blocks trying to
> acquire a lock that the master held before the start of parallelism,
> it will wait forever, even if the master never acquires a lock. But
> if the worker blocks trying to acquire a lock that the master acquired
> *during* parallelism, it's presumably something like a catalog lock or
> a tuple lock or a relation extension lock that the master is going to
> release quickly.

If there's one lock that's acquired that way, there can easily be
two. And then they just need need to be acquired in an inconsistent
order and you have a deadlock.

> > There'll be many more of these, and we can't really
> > reason about them because we used to working locks.
>
> FUD.

It certainly scares me.

> >> 3. I welcome proposals for other ways of handling this problem, even
> >> if they restrict the functionality that can be offered. For example,
> >> a proposal that would make parallel_count revert to single-threaded
> >> mode but terminate without an indefinite wait would be acceptable to
> >> me, provided that it offers some advantage in safety and security over
> >> what I've already got.
> >
> > I think the above example where we only grant the locks required for a
> > specific thing and only on the relevant severity would already be a big
> > improvement. Even better would be to to use the computed set of locks to
> > check whether workers could acquire them and refuse paralellism in that
> > case.
>
> I think that will just produce lots of very-hard-to-debug undetected
> deadlocks and huge volumes of code that tries and fails to assess what
> locks the worker will need.

Teaching the deadlock to recognize that

> > Another thing to do is to mark the lock, both in the master and workers,
> > as not effectively being of the original severity anymore. If the own
> > process again tries to acquire the lock on a heavier severity than what
> > was needed at the time of query execution, error out. At least until
> > parallel mode has confirmed to have ended. That should pretty much never
> > happen.
>
> I'm not sure what you mean by the "severity" of the lock.

The lock level.

Greetings,

Andres Freund

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