On Thu, Jun 30, 2016 at 12:04 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@gmail.com> writes: >> Here is a new patch addressing (I hope) the above comments and a few >> other things. > > Some more comments: > > * It seems like the initialization of root->parallelModeNeeded is still > overcomplicated and/or underexplained. Why do you not merely set it false > to start with, and allow it to become true when/if a Gather is added to > the plan? AFAICS its only point is to tell the executor that there's a > Gather someplace.
That's pretty much right, but there are two conceptually separate things. The first is whether or not we actually attempt to spawn workers, and the second is whether or not we enter parallel mode. Entering parallel mode enables various restrictions that make spawning workers safe, so we cannot spawn workers unless we enter parallel mode. We can, however, enter parallel mode without spawning any workers, and force_parallel_mode=on does so. The point of that is that even if the plan doesn't end up being run inside of a worker, it will be run with most of the same restrictions on what it can do that would be in place if a truly parallel plan were chosen. So we basically do exactly what you are proposing here when force_parallel_mode=off, but when it's turned on then we, uh, force parallel mode. Actually, the original remit of force_parallel_mode was precisely that: force parallel mode. The behavior of also forcing the plan to run inside of a single-copy Gather mode is an additional behavior that was added later on in the development process, but having two separate GUCs felt like overkill. I believe I discussed this on-list with Noah at the time. It's a separate issue from what this patch is about, at any rate. > * typo in new comment in grouping_planner: "final_rel is can be marked" > > + * If the input relation is not parallel-safe, then the window > relation > + * can't be parallel-safe, either. Otherwise, we need to examine the > + * target list and windows for non-parallel-safe constructs. > + * (XXX: Do we also need to check wflists?) > > No, we don't, because the wflists just contain windowfuncs extracted from > the target list. But I'd say "target list and active windows for..." for > more clarity (the point being it's okay to ignore any unused window > clauses). Thanks, fixed. > * root->parallelModeOK is probably not being consulted in as many places > as it would be useful; could we use it to short-circuit any more > has_parallel_hazard tests? I think it short-circuits essentially all of them, because set_rel_consider_parallel() is not called if it's false. If the baserel's consider_parallel flag is false, that will force every other flag to be false as you move up the tree. We don't need to recheck it in join relations or upper relations because those have input relations which should never be consider_parallel unless root->parallelModeOK is set. If there's ever a case where any consider_parallel flag is true when root->parallelModeOK is false, that's a bug. > * I'm still not real happy with the has_parallel_hazard test added to > apply_projection_to_path, and here is why not: if the target path is > not projection-capable, we'll never get to that test. We just pass > the problem off to create_projection_path, which looks no further > than rel->consider_parallel. It seems like we have to add a > has_parallel_hazard test there as well, which is a tad annoying, > not least because all the direct callers of create_projection_path > seem to have made the test already. Thanks for noticing that problem; I've fixed it by inserting a test into create_projection_path. As far as the annoyance factor is concerned, you obviously know that there was a flag there to reduce the cost of that, but you insisted on removing it. Maybe you should consider putting it back. > This gets back to the question of whether rel->consider_parallel is even > useful at the level of upperrels. As far as I can tell, that flag is > really little more than a crutch to let scan/join paths be marked parallel > safe or not with minimum ado. I would rather like to get to a point where > consider_parallel is not used for upperrels, because that avoids the > problem that you're not setting it early enough to be useful for > GetForeignUpperPaths(). (After that I'd like to get to a point where we > don't have it at all, but that may not happen for 9.6.) Well, the point of consider_parallel is that there are some things that are specific to the individual path, and there are some that are properties of the RelOptInfo. It seems highly desirable to check things that fall into the latter category exactly once, rather than once per path. You seem to be fighting hard against that idea, and I'm pretty baffled as to why. That flag avoids a significant amount of unnecessary recomputation in baserels, in joinrels, and in some though not all upperrels, so I'm completely mystified by your urge to eliminate it. If we did eliminate it and relied only on the per-path flags, then we'd end up continually rechecking has_parallel_hazard on a lot more things. For example, right now, we only need to check that the join quals are parallel-safe if the rels from which the join is being formed are parallel-safe. If we already know that one of the input rels isn't parallel-safe, then there's no point in checking the join quals. If we rely on the per-path flags, then it's not obvious whether or not to bother checking the join quals. Later on, when we reach e.g. match_unsorted_outer(), if we already know that there's no hope of generating any parallel path for this relation, we don't need to bother calling consider_parallel_nestloop() to loop over each partial path for the inner rel only to find that none of them work. Now, admittedly, an O(inner partial paths * outer parameterized paths) computation that checks one flag in the parameterized path on every loop and then falls out is probably not that expensive, but I see no point in spitting on such optimizations. I think that flag is more than pulling its own weight, and I'd appreciate a better explanation of why you don't like it. It doesn't cost any more than testing root->parallelModeOK but it lets us skip meaningful work in a lot more cases. >> Unfortunately, I can't think of a clean way to make the "right" thing >> happen here. In general, it's right to prefer a parallel-safe plan >> over an ever-so-slightly more expensive plan that isn't, because that >> might let us join it to a partial path later on. However, if we're at >> the very top of the plan tree, that argument holds no force, so we >> could try to install some kind of exception for that case. That seems >> like it would be fairly invasive and fairly ugly, though. It seems >> like a better option would be to try to kludge the test case somehow >> so that the backward index scan looks at least 1% cheaper than the >> forward index scan, but I can't see how to do that. I don't see, for >> example, that changing any of the usual costing factors would help >> much. We could do something very specialized here, like adding a GUC >> that makes MinMaxAggPath always win, but that seems ugly, too. > > If we made add_path not consider parallel safety as a preference item > when glob->parallelModeOK is false, then we could set > max_parallel_workers_per_gather to zero for this test case and the > problem would go away. In general, that seems like it would be a good > thing for end users too: they could avoid plan changes that are made > on the basis of completely irrelevant parallel-safety. It might be > that we don't actually need an explicit check for that in add_path, > as long as we make sure that no path ever gets marked parallel_safe > when parallelModeOK is false. Ah! Setting max_parallel_workers_per_gather=0 is a very good idea, and it fixes the problem because the rule you postulate is already enforced. As explained above, parallelModeOK is false, then consider_parallel will be false for every rel, and that in turn makes parallel_safe false for every path. Updated patch attached. (Official status update: I'll commit this, possibly over the long weekend or in any event no later than Tuesday, unless there are further review comments before then, in which case I will post a further official status update no later than Tuesday.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
upperrel-consider-parallel-v3.patch
Description: invalid/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
