Re: parallel mode and parallel contexts

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: parallel mode and parallel contexts
Date: 2015-03-06 15:16:12
Message-ID: CA+TgmobqVqKfu+A1rWEVGi7KCyN8tJQirNbqOYs4bu94-9iq8g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Feb 17, 2015 at 11:01 AM, Andres Freund <andres(at)2ndquadrant(dot)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

Attachment Content-Type Size
parallel-mode-v7.patch binary/octet-stream 138.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2015-03-06 15:16:59 Re: Clamping reulst row number of joins.
Previous Message Tom Lane 2015-03-06 15:07:53 Re: Clamping reulst row number of joins.