On 6 January 2015 at 16:33, Robert Haas <robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers