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. * 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers