Обсуждение: fixing consider_parallel for upper planner rels
On Sun, Jun 26, 2016 at 10:42 PM, Noah Misch <noah@leadboat.com> wrote: > On Mon, Jun 13, 2016 at 05:27:13PM -0400, Robert Haas wrote: >> One problem that I've realized that is related to this is that the way >> that the consider_parallel flag is being set for upper rels is almost >> totally bogus, which I believe accounts for your complaint at PGCon >> that force_parallel_mode was not doing as much as it ought to do. >> When I originally wrote a lot of this logic, there were no upper rels, >> which led to this code: >> >> if (force_parallel_mode == FORCE_PARALLEL_OFF || !glob->parallelModeOK) >> { >> glob->parallelModeNeeded = false; >> glob->wholePlanParallelSafe = false; /* either >> false or don't care */ >> } >> else >> { >> glob->parallelModeNeeded = true; >> glob->wholePlanParallelSafe = >> !has_parallel_hazard((Node *) parse, false); >> } >> >> The main way that wholePlanParallelSafe is supposed to be cleared is >> in create_plan: >> >> /* Update parallel safety information if needed. */ >> if (!best_path->parallel_safe) >> root->glob->wholePlanParallelSafe = false; >> >> However, at the time that code was written, it was impossible to rely >> entirely on the latter mechanism. Since there were no upper rels and >> no paths for upper plan nodes, you could have the case where every >> path was parallel-safe but the whole plan was node. Therefore the >> code shown above was needed to scan the whole darned plan for >> parallel-unsafe things. Post-pathification, this whole thing is >> pretty bogus: upper rels mostly don't get consider_parallel set at >> all, which means plans involving upper rels get marked parallel-unsafe >> even if they are not, which means the wholePlanParallelSafe flag gets >> cleared in a bunch of cases where it shouldn't. And on the flip side >> I think that the first chunk of code quoted above would be totally >> unnecessary if we were actually setting consider_parallel correctly on >> the upper rels. > > [Action required within 72 hours. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 9.6 open item ("set > consider_parallel correctly for upper rels"). Robert, since you committed the > patch believed to have created it, you own this open item. If some other > commit is more relevant or if this does not belong as a 9.6 open item, please > let us know. Otherwise, please observe the policy on open item ownership[1] > and send a status update within 72 hours of this message. Include a date for > your subsequent status update. Testers may discover new open items at any > time, and I want to plan to get them all fixed well in advance of shipping > 9.6rc1. Consequently, I will appreciate your efforts toward speedy > resolution. Thanks. I'm not sure how to proceed here. I have asked Tom several times to look at the WIP patch and offer comments, but he so far has not done so. I am reluctant to commit more patches whacking the planner around post-beta2 without some review from the guy who invented the upper planner pathification stuff that broke this in the first place. What I have here was more or less correct before that. It could be argued that this problem should really be attributed to 3fc6e2d7f5b652b417fa6937c34de2438d60fa9f rather than any of the parallel query commits -- though it's certainly the case that e06a38965b3bcdaa881e7e06892d4d8ab6c2c980 was the result of massive fuzzy thinking on my part. I am quite worried that if I whack this around some more it's going to break more stuff, and I don't know that I feel very comfortable doing that without some independent review. Suggestions? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I'm not sure how to proceed here. I have asked Tom several times to > look at the WIP patch and offer comments, but he so far has not done > so. Oh, I thought you were still actively working on it. What patch do you want me to review? regards, tom lane
On Mon, Jun 27, 2016 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not sure how to proceed here. I have asked Tom several times to >> look at the WIP patch and offer comments, but he so far has not done >> so. > > Oh, I thought you were still actively working on it. What patch do > you want me to review? I'm looking for an opinion on the WIP patch attached to: https://www.postgresql.org/message-id/CA+TgmoZwJB9EaiBj-MEeAQ913WkKOz4aQ7VQnCfrDLs9WYhZdQ@mail.gmail.com It may not be correct in detail, but I'd like to know whether you think it's going in the generally correct direction and what major concerns you might have before spending more time on it. Also, I'd like to know whether you think it's something we should try to put into 9.6 or whether you think we should leave it for next cycle. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jun 27, 2016 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Oh, I thought you were still actively working on it. What patch do >> you want me to review? > I'm looking for an opinion on the WIP patch attached to: > https://www.postgresql.org/message-id/CA+TgmoZwJB9EaiBj-MEeAQ913WkKOz4aQ7VQnCfrDLs9WYhZdQ@mail.gmail.com OK, I took a quick look. Some of the details are obsolete due to my over-the-weekend hacking on parallel aggregation, but I think generally you have the right idea that we should set upperrel consider_parallel flags on the basis of "input rel has consider_parallel = true AND there are no parallel hazards in operations to be performed at this level". A few specific comments: * Can't we remove wholePlanParallelSafe altogether, in favor of just examining best_path->parallel_safe in standard_planner? * In grouping_planner, I'm not exactly convinced that final_rel->consider_parallel can just be copied up without any further restrictions. Easiest counterexample is a parallel restricted function in LIMIT. * Why have the try_parallel_path flag at all in create_grouping_paths, rather than just setting or not setting grouped_rel->consider_parallel? If your thinking is that this is dealing with implementation restrictions that might not apply to paths added by an extension, then OK, but the variable needs to have a less generic name. Maybe try_parallel_aggregation. * Copy/paste error in comment in create_distinct_paths: s/window/distinct/ * Not following what you did to apply_projection_to_path, and the comment therein isn't helping. > It may not be correct in detail, but I'd like to know whether you > think it's going in the generally correct direction and what major > concerns you might have before spending more time on it. Also, I'd > like to know whether you think it's something we should try to put > into 9.6 or whether you think we should leave it for next cycle. I think we should try to get this right in 9.6. For one thing, the more stuff we can easily exercise in parallel mode, the more likely we are to find bugs before they reach the field. regards, tom lane
On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jun 27, 2016 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Oh, I thought you were still actively working on it. What patch do >>> you want me to review? > >> I'm looking for an opinion on the WIP patch attached to: >> https://www.postgresql.org/message-id/CA+TgmoZwJB9EaiBj-MEeAQ913WkKOz4aQ7VQnCfrDLs9WYhZdQ@mail.gmail.com > > OK, I took a quick look. Some of the details are obsolete due to my > over-the-weekend hacking on parallel aggregation, but I think generally > you have the right idea that we should set upperrel consider_parallel > flags on the basis of "input rel has consider_parallel = true AND there > are no parallel hazards in operations to be performed at this level". OK, great. Thanks. > A few specific comments: > > * Can't we remove wholePlanParallelSafe altogether, in favor of just > examining best_path->parallel_safe in standard_planner? Yes, I think we can. Before the upper planner used paths, we needed a way to get wholePlanParallelSafe = false even if every generated path was parallel-safe, but now that all planning stages have paths, we can get rid of that. > * In grouping_planner, I'm not exactly convinced that > final_rel->consider_parallel can just be copied up without any further > restrictions. Easiest counterexample is a parallel restricted function in > LIMIT. OK, will look. > * Why have the try_parallel_path flag at all in create_grouping_paths, > rather than just setting or not setting grouped_rel->consider_parallel? > If your thinking is that this is dealing with implementation restrictions > that might not apply to paths added by an extension, then OK, but the > variable needs to have a less generic name. Maybe try_parallel_aggregation. Suppose all of the relevant functions are parallel-safe, but the aggregates don't have combine functions. In that case, consider_parallel ought to be true, because parallel strategies are OK as a general matter, but the one and only parallel strategy we have today - two-phase aggregation - will not work. > * Copy/paste error in comment in create_distinct_paths: s/window/distinct/ OK, will fix. > * Not following what you did to apply_projection_to_path, and the comment > therein isn't helping. Gee, I wonder why not? :-) The basic problem here is that applying a projection to a path can render a formerly parallel-safe path no longer parallel-safe. If we jam a parallel-restricted target list into a formerly parallel-safe path, we'd better also clear path->parallel_safe. Currently, apply_projection_to_path only needs to call has_parallel_hazard for an input which is a GatherPath, which isn't too expensive because most paths are not GatherPaths and if we get one that is, well, we can hope parallel query will win enough during execution to make up for the extra planning cost. But if we want the consider_parallel and parallel_safe flags to be set correctly for all upper rels and paths, it seems that we need to do it always - hence the dismayed comment. Thoughts? >> It may not be correct in detail, but I'd like to know whether you >> think it's going in the generally correct direction and what major >> concerns you might have before spending more time on it. Also, I'd >> like to know whether you think it's something we should try to put >> into 9.6 or whether you think we should leave it for next cycle. > > I think we should try to get this right in 9.6. For one thing, the > more stuff we can easily exercise in parallel mode, the more likely > we are to find bugs before they reach the field. OK. (Official status update: I will post an updated version of this patch by Thursday.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> * Not following what you did to apply_projection_to_path, and the comment >> therein isn't helping. > Gee, I wonder why not? :-) > The basic problem here is that applying a projection to a path can > render a formerly parallel-safe path no longer parallel-safe. If we > jam a parallel-restricted target list into a formerly parallel-safe > path, we'd better also clear path->parallel_safe. Currently, > apply_projection_to_path only needs to call has_parallel_hazard for an > input which is a GatherPath, which isn't too expensive because most > paths are not GatherPaths and if we get one that is, well, we can hope > parallel query will win enough during execution to make up for the > extra planning cost. But if we want the consider_parallel and > parallel_safe flags to be set correctly for all upper rels and paths, > it seems that we need to do it always - hence the dismayed comment. > Thoughts? Seems to me that it should generally be the case that consider_parallel would already be clear on the parent rel if the tlist isn't parallel safe, and if it isn't we probably have a bug elsewhere. If it makes you feel better, maybe you could add Assert(!has_parallel_hazard(...)) here? I guess this means that apply_projection_to_path would need to clear parallel_safe if rel->consider_parallel isn't true. This would correspond to situations where we are taking a parallel-safe path for a lower-level relation that has consider_parallel true, and repurposing it for a new upperrel that has consider_parallel false. Maybe it'd be better to not do that but just force use of a separate ProjectionPath if the parallel_safe flag needs to change. (I think 8b9d323cb may have made this a little less messy than it was when you did your draft patch, btw.) regards, tom lane
On Mon, Jun 27, 2016 at 5:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> * Not following what you did to apply_projection_to_path, and the comment >>> therein isn't helping. > >> Gee, I wonder why not? :-) > >> The basic problem here is that applying a projection to a path can >> render a formerly parallel-safe path no longer parallel-safe. If we >> jam a parallel-restricted target list into a formerly parallel-safe >> path, we'd better also clear path->parallel_safe. Currently, >> apply_projection_to_path only needs to call has_parallel_hazard for an >> input which is a GatherPath, which isn't too expensive because most >> paths are not GatherPaths and if we get one that is, well, we can hope >> parallel query will win enough during execution to make up for the >> extra planning cost. But if we want the consider_parallel and >> parallel_safe flags to be set correctly for all upper rels and paths, >> it seems that we need to do it always - hence the dismayed comment. >> Thoughts? > > Seems to me that it should generally be the case that consider_parallel > would already be clear on the parent rel if the tlist isn't parallel safe, > and if it isn't we probably have a bug elsewhere. If it makes you feel > better, maybe you could add Assert(!has_parallel_hazard(...)) here? I don't see that this is true. If someone does SELECT pg_backend_pid() FROM pgbench_accounts, there's only one RelOptInfo and nothing to clear consider_parallel for it anywhere else. I've really been wondering whether there ought to be a separate UPPERREL_FOO whose only purpose is to handle applying scanjoin_target to the topmost scan/join rel. Effectively, the current code is trying to do that transformation in place. We start with a scan/join rel that emits whatever it naturally emits and then frob all of the paths and partial paths to emit the scanjoin_target. But this seems quite cumbersome. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jun 27, 2016 at 5:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Seems to me that it should generally be the case that consider_parallel >> would already be clear on the parent rel if the tlist isn't parallel safe, >> and if it isn't we probably have a bug elsewhere. If it makes you feel >> better, maybe you could add Assert(!has_parallel_hazard(...)) here? > I don't see that this is true. If someone does SELECT > pg_backend_pid() FROM pgbench_accounts, there's only one RelOptInfo > and nothing to clear consider_parallel for it anywhere else. Huh? The final tlist would go with the final_rel, ISTM, not the scan relation. Maybe we have some rejiggering to do to make that true, though. > I've really been wondering whether there ought to be a separate > UPPERREL_FOO whose only purpose is to handle applying scanjoin_target > to the topmost scan/join rel. That's another way of thinking about it, I guess. > Effectively, the current code is trying > to do that transformation in place. We start with a scan/join rel > that emits whatever it naturally emits and then frob all of the paths > and partial paths to emit the scanjoin_target. But this seems quite > cumbersome. It'd be no less cumbersome, because you'd end up with all those same paths surviving into the FOO upperrel. But it might be logically cleaner. (Thinks for a bit...) Actually, scratch that. It was very intentional on my part that different Paths for the same relation might produce different tlists. Without that, there's no way that we're going to get a solution that allows extracting index expression values from index-only scans in nontrivial plans. Otherwise I wouldn't have introduced PathTarget to begin with, because we already had a perfectly good way of representing a one-size-fits-all result tlist for each RelOptInfo. So it seems like we should not introduce a dependency here that assumes that all Paths for a given rel have equivalent parallel_safe settings. Maybe it'd be okay for the special case of index expressions, because they are almost certainly going to be parallel safe, but in general it's a restriction we don't want. You could still save something by writing code along the line ofif (path->parallel_safe && has_parallel_hazard(...)) path->parallel_safe = false; so as not to run has_parallel_hazard in the case where we already know we lost. Doing more than this would probably involve caching parallel-safety status in PathTarget itself, which is certainly doable but we should measure first to see if it's worth the trouble. regards, tom lane
On Mon, Jun 27, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jun 27, 2016 at 5:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Seems to me that it should generally be the case that consider_parallel >>> would already be clear on the parent rel if the tlist isn't parallel safe, >>> and if it isn't we probably have a bug elsewhere. If it makes you feel >>> better, maybe you could add Assert(!has_parallel_hazard(...)) here? > >> I don't see that this is true. If someone does SELECT >> pg_backend_pid() FROM pgbench_accounts, there's only one RelOptInfo >> and nothing to clear consider_parallel for it anywhere else. > > Huh? The final tlist would go with the final_rel, ISTM, not the scan > relation. Maybe we have some rejiggering to do to make that true, though. Mumble. You're right that there are two rels involved, but I think I'm still right about the substance of the problem. I can't tell whether the remainder of your email concedes that point or whether we're still in disagreement. In that example, SELECT pg_backend_pid() FROM pgbench_accounts, we're first going to form a path for scanning pgbench_accounts. The rel for pgbench_accounts will be marked parallel_safe because it's just a scan of a relation outputting some number (possibly 0) of Vars. That rel becomes the final scan/join rel, and the path or paths for that rel are parallel-safe. Now, when we apply the final tlist to those paths, they are no longer parallel-safe. apply_projection_to_path() has got to realize that. > You could still save something by writing code along the line of > if (path->parallel_safe && > has_parallel_hazard(...)) > path->parallel_safe = false; > so as not to run has_parallel_hazard in the case where we already know > we lost. I agree, and that does seem worth doing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Jun 27, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Huh? The final tlist would go with the final_rel, ISTM, not the scan >> relation. Maybe we have some rejiggering to do to make that true, though. > Mumble. You're right that there are two rels involved, but I think > I'm still right about the substance of the problem. I can't tell > whether the remainder of your email concedes that point or whether > we're still in disagreement. Well, I was trying to find a way that we could rely on the rel's consider_parallel marking rather than having to test the pathtarget as such, but I concluded that we couldn't do that. Sorry if thinking out loud confused you. regards, tom lane
On Wed, Jun 29, 2016 at 1:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Jun 27, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Huh? The final tlist would go with the final_rel, ISTM, not the scan >>> relation. Maybe we have some rejiggering to do to make that true, though. > >> Mumble. You're right that there are two rels involved, but I think >> I'm still right about the substance of the problem. I can't tell >> whether the remainder of your email concedes that point or whether >> we're still in disagreement. > > Well, I was trying to find a way that we could rely on the rel's > consider_parallel marking rather than having to test the pathtarget as > such, but I concluded that we couldn't do that. Sorry if thinking > out loud confused you. OK, no problem. I was arguing from the beginning that we couldn't make that work, so it sounds like we are now in agreement. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jun 27, 2016 at 4:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > A few specific comments: > > * Can't we remove wholePlanParallelSafe altogether, in favor of just > examining best_path->parallel_safe in standard_planner? > > * In grouping_planner, I'm not exactly convinced that > final_rel->consider_parallel can just be copied up without any further > restrictions. Easiest counterexample is a parallel restricted function in > LIMIT. > > * Why have the try_parallel_path flag at all in create_grouping_paths, > rather than just setting or not setting grouped_rel->consider_parallel? > If your thinking is that this is dealing with implementation restrictions > that might not apply to paths added by an extension, then OK, but the > variable needs to have a less generic name. Maybe try_parallel_aggregation. > > * Copy/paste error in comment in create_distinct_paths: s/window/distinct/ > > * Not following what you did to apply_projection_to_path, and the comment > therein isn't helping. Here is a new patch addressing (I hope) the above comments and a few other things. Unfortunately, it causes the by-now-familiar regression test failure in the aggregates test: *** /Users/rhaas/pgsql/src/test/regress/expected/aggregates.out 2016-06-29 20:04:03.000000000 -0400 --- /Users/rhaas/pgsql/src/test/regress/results/aggregates.out 2016-06-29 20:06:28.000000000 -0400 *************** *** 577,590 **** explain (costs off) select max(unique1) from tenk1 where unique1 > 42000; ! QUERY PLAN ! --------------------------------------------------------------------------- ! Result ! InitPlan 1 (returns $0) ! -> Limit ! -> Index Only Scan Backward using tenk1_unique1 on tenk1 ! Index Cond: ((unique1 IS NOT NULL) AND (unique1 > 42000)) ! (5 rows) select max(unique1) from tenk1 where unique1 > 42000; max --- 577,588 ---- explain (costs off) select max(unique1) from tenk1 where unique1 > 42000; ! QUERY PLAN ! ---------------------------------------------------- ! Aggregate ! -> Index Only Scan using tenk1_unique1 on tenk1 ! Index Cond: (unique1 > 42000) ! (3 rows) select max(unique1) from tenk1 where unique1 > 42000; max ====================================================================== The problem, of course, is that scanning tenk1_unique1 forward to find 0 rows at the end of the index and scanning it backward to find 0 rows at the end of the index cost almost exactly the same amount, which is probably a good thing when you stop to think about it. On my system, the expected plan costs 4.31 and the plan that actually shows up costs 4.32, so we end up picking the latter because, since it does not involve subqueries, it's parallel-safe. It's tempting to remove the test on the theory that there's actually little point in verifying which of two essentially-equivalent plans get chosen, but I think that's probably missing the point: the next test runs the same query without EXPLAIN, presumably to verify that the MinMaxAggPath approach gets the right *answer* when there are no matching rows, and in order for that test to mean anything, we need to first verify that we're actually getting the plan whose execution-time behavior we want to check. 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. Suggestions? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@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. * 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). * 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'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. 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.) > 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. regards, tom lane
BTW, I just had another thought about reducing the cost of has_parallel_hazard checks, to wit: you already made one pass over the entire query to verify that there's no PARALLEL UNSAFE functions anywhere. If that pass were to also track whether there are any PARALLEL RESTRICTED functions anywhere, then in very many common cases, subsequent tests on portions of the query would not have to do anything, because we'd already know there was nothing to worry about. regards, tom lane
On Thu, Jun 30, 2016 at 12:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@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
Вложения
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Jun 30, 2016 at 12:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> * It seems like the initialization of root->parallelModeNeeded is still >> overcomplicated and/or underexplained. > 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. And the point of that is what, exactly? If the only change is that "some restrictions get enforced", I am not clear on why we need such a test mode in cases where the planner is afraid to put a top Gather on the plan. In particular, given the coding as you now have it, it seems like the only case where there's any difference is where we set glob->parallelModeOK but nonetheless end up with a not-parallel-safe topmost path (that doesn't have a Gather within it). It's not clear to me why having the executor switch into parallel mode makes sense at all with such a plan. >> * 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. No, that flag was on apply_projection_to_path, and I didn't care for it because most of the callers didn't appear to want to go to the trouble of setting it correctly. Adding such an argument to create_projection_path as well doesn't seem to make that better. > 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. What I'm not happy about is that as you've got things constituted, the GetForeignUpperPaths hook is broken so far as injecting parallel paths is concerned, because the consider_parallel flags for the upperrels aren't set yet when it gets called. If we keep consider_parallel with its present usage, we're going to have to refactor things to fix that. > 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.) Don't have time to re-read this right now, but maybe tomorrow or Saturday. regards, tom lane
On Thu, Jun 30, 2016 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > BTW, I just had another thought about reducing the cost of > has_parallel_hazard checks, to wit: you already made one pass over the > entire query to verify that there's no PARALLEL UNSAFE functions anywhere. > If that pass were to also track whether there are any PARALLEL RESTRICTED > functions anywhere, then in very many common cases, subsequent tests on > portions of the query would not have to do anything, because we'd already > know there was nothing to worry about. Yeah, that's true. I'm not sure how much those has_parallel_hazard() checks are costing us. The ones on quals seem like they are probably pretty cheap, because if you've got enough quals for the cycles we spend checking them to matter, it's the proliferation of paths and RelOptInfos that is going to kill you, not the cost of the has_parallel_hazard() checks. I think, anyway. The ones on target lists, which I didn't foresee, seem more troubling, because you could easily be selecting a large number of columns and so we end up with lots of RelOptInfos that all have long target lists and we've got to keep checking those target lists over and over again. I'd like to find a way to do better there. I still think that it might be better to optimize the tlist-is-all-vars case instead of doing what you propose here, but I'm not sure, and your intuition may well be better than mine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 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. > > And the point of that is what, exactly? If the only change is that > "some restrictions get enforced", I am not clear on why we need such > a test mode in cases where the planner is afraid to put a top Gather on > the plan. In particular, given the coding as you now have it, it seems > like the only case where there's any difference is where we set > glob->parallelModeOK but nonetheless end up with a not-parallel-safe > topmost path (that doesn't have a Gather within it). It's not clear > to me why having the executor switch into parallel mode makes sense at > all with such a plan. Suppose you create a PL/pgsql function that does an UPDATE and mark it PARALLEL RESTRICTED. You wonder whether you've marked it correctly. You can set force_parallel_mode=on and SELECT myfunc(). The subsequent ERROR tells you that you've mismarked it. >>> * 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. > > No, that flag was on apply_projection_to_path, and I didn't care for it > because most of the callers didn't appear to want to go to the trouble of > setting it correctly. Adding such an argument to create_projection_path > as well doesn't seem to make that better. I'm open to suggestions. As I've noted a few times already, though maybe less clearly than I should have done, I think it's quite likely to be a good idea to try to avoid the overhead of running has_parallel_hazard repeatedly on the same tlists, or for that matter, running it on tlists at all. I don't have any evidence that's expensive enough to cost, just a hunch. Exactly how to do that best, I'm not sure. >> 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. > > What I'm not happy about is that as you've got things constituted, > the GetForeignUpperPaths hook is broken so far as injecting parallel paths > is concerned, because the consider_parallel flags for the upperrels > aren't set yet when it gets called. If we keep consider_parallel with > its present usage, we're going to have to refactor things to fix that. I see. It seems to me, and I may be failing to understand something, that the placement of create_upper_paths_hook is substantially better than the placement of GetForeignUpperPaths. If the latter were moved to where the former now is, then consider_parallel would be set sufficiently early and everything would be fine. We could alternatively fish out the code to set consider_parallel for the upper rels and do all of that work before calling that hook. That's a bit hairy, because we'd basically go set all of the consider_parallel flags, then call that hook, then circle back around for the core path generation, but I don't see any intrinsic obstacle to that line of attack. I'm not very sure that one call for all upper rels is going to be convenient, though. >> 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.) > > Don't have time to re-read this right now, but maybe tomorrow or > Saturday. OK, thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> And the point of that is what, exactly? If the only change is that >> "some restrictions get enforced", I am not clear on why we need such >> a test mode in cases where the planner is afraid to put a top Gather on >> the plan. In particular, given the coding as you now have it, it seems >> like the only case where there's any difference is where we set >> glob->parallelModeOK but nonetheless end up with a not-parallel-safe >> topmost path (that doesn't have a Gather within it). It's not clear >> to me why having the executor switch into parallel mode makes sense at >> all with such a plan. > Suppose you create a PL/pgsql function that does an UPDATE and mark it > PARALLEL RESTRICTED. You wonder whether you've marked it correctly. > You can set force_parallel_mode=on and SELECT myfunc(). The > subsequent ERROR tells you that you've mismarked it. Right, but that statement is still true with the logic I'm imagining. I would also argue that the existing text in config.sgml explaining what this parameter does corresponds much more nearly to what I'm suggesting than to what you say the semantics are. >> What I'm not happy about is that as you've got things constituted, >> the GetForeignUpperPaths hook is broken so far as injecting parallel paths >> is concerned, because the consider_parallel flags for the upperrels >> aren't set yet when it gets called. If we keep consider_parallel with >> its present usage, we're going to have to refactor things to fix that. > I see. It seems to me, and I may be failing to understand something, > that the placement of create_upper_paths_hook is substantially better > than the placement of GetForeignUpperPaths. If the latter were moved > to where the former now is, then consider_parallel would be set > sufficiently early and everything would be fine. Yeah, I came to more or less the same conclusion last night. Will see to it after you commit this. regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Don't have time to re-read this right now, but maybe tomorrow or >> Saturday. > OK, thanks. There's still the extra-word problem here: + * If the input rel is marked consider_parallel and there's nothing + * that's not parallel-safe in the LIMIT clause, then the final_rel is + * can be marked consider_parallel as well. Other than that, and the quibble over initialization of parallelModeNeeded, I'm good with this. regards, tom lane
On Fri, Jul 1, 2016 at 11:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Don't have time to re-read this right now, but maybe tomorrow or >>> Saturday. > >> OK, thanks. > > There's still the extra-word problem here: > > + * If the input rel is marked consider_parallel and there's nothing > + * that's not parallel-safe in the LIMIT clause, then the final_rel is > + * can be marked consider_parallel as well. > > Other than that, and the quibble over initialization of > parallelModeNeeded, I'm good with this. OK, committed. I think we can argue about parallelModeNeeded as a separate matter. That's merely a sideshow as far as this patch is concerned. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Jul 1, 2016 at 10:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> And the point of that is what, exactly? If the only change is that >>> "some restrictions get enforced", I am not clear on why we need such >>> a test mode in cases where the planner is afraid to put a top Gather on >>> the plan. In particular, given the coding as you now have it, it seems >>> like the only case where there's any difference is where we set >>> glob->parallelModeOK but nonetheless end up with a not-parallel-safe >>> topmost path (that doesn't have a Gather within it). It's not clear >>> to me why having the executor switch into parallel mode makes sense at >>> all with such a plan. > >> Suppose you create a PL/pgsql function that does an UPDATE and mark it >> PARALLEL RESTRICTED. You wonder whether you've marked it correctly. >> You can set force_parallel_mode=on and SELECT myfunc(). The >> subsequent ERROR tells you that you've mismarked it. > > Right, but that statement is still true with the logic I'm imagining. > I would also argue that the existing text in config.sgml explaining > what this parameter does corresponds much more nearly to what I'm > suggesting than to what you say the semantics are. I just went and reread that description and it looks to me like it matches what I said. I guess I don't really understand what exactly you want to change. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company