Re: Unneeded parallel safety tests in grouping_planner
(2019/02/28 0:46), Robert Haas wrote: On Wed, Feb 27, 2019 at 7:46 AM Etsuro Fujita wrote: Yet another thing I noticed while working on [1] is this in grouping_planner: /* * 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 can be marked * consider_parallel as well. Note that if the query has rowMarks or is * not a SELECT, consider_parallel will be false for every relation in the * query. */ if (current_rel->consider_parallel&& is_parallel_safe(root, parse->limitOffset)&& is_parallel_safe(root, parse->limitCount)) final_rel->consider_parallel = true; If there is a need to add a LIMIT node, we don't consider generating partial paths for the final relation below (see commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary anymore to assess the parallel-safety of the LIMIT and OFFSET clauses. To save cycles, why not remove those tests from that function like the attached? Because in the future we might want to consider generating partial_paths in cases where we don't do so today. I repeatedly made the mistake of believing that I could not bother setting consider_parallel entirely correctly for one reason or another, and I've gone through multiple iterations of fixing cases where I did that and it turned out to cause problems. I now believe that we should try to get it right in every case, whether or not we currently think it's possible for it to matter. Sometimes it matters in ways that aren't obvious, and it complicates further development. I don't think we'd save much by changing this test anyway. Those is_parallel_safe() tests aren't entirely free, of course, but they should be very cheap. I got the point. Thanks for the explanation! Best regards, Etsuro Fujita
Re: Unneeded parallel safety tests in grouping_planner
On Wed, Feb 27, 2019 at 7:46 AM Etsuro Fujita wrote: > Yet another thing I noticed while working on [1] is this in > grouping_planner: > > /* > * 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 can be > marked > * consider_parallel as well. Note that if the query has rowMarks or is > * not a SELECT, consider_parallel will be false for every relation > in the > * query. > */ > if (current_rel->consider_parallel && > is_parallel_safe(root, parse->limitOffset) && > is_parallel_safe(root, parse->limitCount)) > final_rel->consider_parallel = true; > > If there is a need to add a LIMIT node, we don't consider generating > partial paths for the final relation below (see commit > 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary > anymore to assess the parallel-safety of the LIMIT and OFFSET clauses. > To save cycles, why not remove those tests from that function like the > attached? Because in the future we might want to consider generating partial_paths in cases where we don't do so today. I repeatedly made the mistake of believing that I could not bother setting consider_parallel entirely correctly for one reason or another, and I've gone through multiple iterations of fixing cases where I did that and it turned out to cause problems. I now believe that we should try to get it right in every case, whether or not we currently think it's possible for it to matter. Sometimes it matters in ways that aren't obvious, and it complicates further development. I don't think we'd save much by changing this test anyway. Those is_parallel_safe() tests aren't entirely free, of course, but they should be very cheap. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Unneeded parallel safety tests in grouping_planner
Hi,
Yet another thing I noticed while working on [1] is this in
grouping_planner:
/*
* 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 can be
marked
* consider_parallel as well. Note that if the query has rowMarks or is
* not a SELECT, consider_parallel will be false for every relation
in the
* query.
*/
if (current_rel->consider_parallel &&
is_parallel_safe(root, parse->limitOffset) &&
is_parallel_safe(root, parse->limitCount))
final_rel->consider_parallel = true;
If there is a need to add a LIMIT node, we don't consider generating
partial paths for the final relation below (see commit
0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52), so it seems unnecessary
anymore to assess the parallel-safety of the LIMIT and OFFSET clauses.
To save cycles, why not remove those tests from that function like the
attached?
Best regards,
Etsuro Fujita
[1] https://commitfest.postgresql.org/22/1950/
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***
*** 2141,2155 grouping_planner(PlannerInfo *root, bool inheritance_update,
final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
/*
! * 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 can be marked
! * consider_parallel as well. Note that if the query has rowMarks or is
! * not a SELECT, consider_parallel will be false for every relation in the
! * query.
*/
! if (current_rel->consider_parallel &&
! is_parallel_safe(root, parse->limitOffset) &&
! is_parallel_safe(root, parse->limitCount))
final_rel->consider_parallel = true;
/*
--- 2141,2152
final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
/*
! * If the input rel is marked consider_parallel and there's no need to add
! * a LIMIT node, then the final_rel can be marked consider_parallel as
! * well. Note that if the query has rowMarks or is not a SELECT,
! * consider_parallel will be false for every relation in the query.
*/
! if (current_rel->consider_parallel && !limit_needed(parse))
final_rel->consider_parallel = true;
/*
***
*** 2263,2272 grouping_planner(PlannerInfo *root, bool inheritance_update,
* Generate partial paths for final_rel, too, if outer query levels might
* be able to make use of them.
*/
! if (final_rel->consider_parallel && root->query_level > 1 &&
! !limit_needed(parse))
{
! Assert(!parse->rowMarks && parse->commandType == CMD_SELECT);
foreach(lc, current_rel->partial_pathlist)
{
Path *partial_path = (Path *) lfirst(lc);
--- 2260,2269
* Generate partial paths for final_rel, too, if outer query levels might
* be able to make use of them.
*/
! if (final_rel->consider_parallel && root->query_level > 1)
{
! Assert(!parse->rowMarks && !limit_needed(parse) &&
! parse->commandType == CMD_SELECT);
foreach(lc, current_rel->partial_pathlist)
{
Path *partial_path = (Path *) lfirst(lc);
