Re: parallel mode and parallel contexts

On Tue, Feb 17, 2015 at 11:01 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> 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.

Did you get a chance to look at this yet?

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

I don't know.  I looked at it more and I don't really see a problem,
but what do I know?

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

Ah, crap.  So, yeah, we need to save and restore that, too.  Fixed in
the attached version.

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

I think that's just to handle things like BEGIN ISOLATION LEVEL
SERIALIZABLE.  A plain BEGIN would work fine without that AFAICS.  It
doesn't seem particularly ugly to me either, but I guess that's
neither here nor there.

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

Well, if you're only going to call built-in functions, and if you
exclude all of the built-in functions that that can take locks on
arbitrary objects like pg_get_object_address() and table_to_xml(), and
if you leave locks on catalog tables aside, then, given further that
we're restricting ourselves to read-only transactions, you *can*
determine the locks that will be required for a query -- there won't
be any.  But one cannot exclude catalog tables by fiat, and all of
those other restrictions are ones that I'd like to have a chance of
relaxing at some point.  It's entirely reasonable for a user to want
to parallelize a query that contains a user-defined PL/pgsql function,
though, and that might do anything.

> 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 is a detailed and hopefully rigorous analysis of locking-related
scenarios in README.parallel in the patch version after the one your
reviewed (posted 2015-02-15).  Have you looked at that?  (It's also in
this version.)

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