From: | Simon Riggs <simon(at)2ndQuadrant(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: parallel mode and parallel contexts |
Date: | 2015-01-06 20:04:04 |
Message-ID: | CA+U5nMKaCpDN1VZ8Om6JFQnZ=6ECHBYpfXqHW5rJSgvePmvEnA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 6 January 2015 at 16:33, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> These comments don’t have any explanation or justification
>
> OK, I rewrote them. Hopefully it's better now.
Thanks for new version and answers.
>> * Transaction stuff strikes me as overcomplicated and error prone.
>> Given there is no explanation or justification for most of it, I’d be
>> inclined to question why its there
>
> Gosh, I was pretty pleased with how simple the transaction integration
> turned out to be. Most of what's there right now is either (a) code
> to copy state from the master to the parallel workers or (b) code to
> throw errors if the workers try to things that aren't safe. I suspect
> there are a few things missing, but I don't see anything there that
> looks unnecessary.
If you can explain it in more detail in comments and README, I may
agree. At present, I don't get it and it makes me nervous.
The comment says
"Only the top frame of the transaction state stack is copied to a parallel
worker"
but I'm not sure why.
Top meaning the current subxact or the main xact?
If main, why are do we need XactTopTransactionId
>> This is very impressive, but it looks like we’re trying to lift too
>> much weight on the first lift. If we want to go anywhere near this, we
>> need to have very clear documentation about how and why its like that.
>> I’m actually very sorry to say that because the review started very
>> well, much much better than most.
>
> When I posted the group locking patch, it got criticized because it
> didn't actually do anything useful by itself; similarly, the
> pg_background patch was criticized for not being a large enough step
> toward parallelism. So, this time, I posted something more
> comprehensive. I don't think it's quite complete yet. I expect a
> committable version of this patch to be maybe another 500-1000 lines
> over what I have here right now -- I think it needs to do something
> about heavyweight locking, and I expect that there are some unsafe
> things that aren't quite prohibited yet. But the current patch is
> only 2300 lines, which is not astonishingly large for a feature of
> this magnitude; if anything, I'd say it's surprisingly small, due to a
> year and a half of effort laying the necessary groundwork via long
> series of preliminary commits. I'm not unwilling to divide this up
> some more if we can agree on a way to do that that makes sense, but I
> think we're nearing the point where we need to take the plunge and
> say, look, this is version one of parallelism. Thunk.
I want this also; the only debate is where to draw the line and please
don't see that as criticism.
I'm very happy it's so short, I agree it could be longer.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Gierth | 2015-01-06 20:21:56 | Re: Final Patch for GROUPING SETS |
Previous Message | Peter Geoghegan | 2015-01-06 19:48:41 | Re: Possible typo in create_policy.sgml |