Re: parallel mode and parallel contexts - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: parallel mode and parallel contexts |
Date | |
Msg-id | CA+TgmobqVqKfu+A1rWEVGi7KCyN8tJQirNbqOYs4bu94-9iq8g@mail.gmail.com Whole thread Raw |
In response to | Re: parallel mode and parallel contexts (Andres Freund <andres@2ndquadrant.com>) |
Responses |
Re: parallel mode and parallel contexts
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: