From 3112cc7e40778d3d377660d864db1f218a3b2cf6 Mon Sep 17 00:00:00 2001 From: David Rowley Date: Tue, 8 Nov 2022 14:36:38 +1300 Subject: [PATCH v3 2/3] Remove pessimistic cost penalization from Incremental Sort When incremental sorts were added in v13 a 1.5x pessimism factor was added to the cost modal. Seemingly this was done because the cost modal only has an estimate of the total number of input rows and the number of presorted groups. It assumes that the input rows will be evenly distributed throughout the presorted groups. The 1.5x pessimism factor was added to slightly reduce the likelihood of incremental sorts being used in the hope to avoid performance regressions where an incremental sort plan was picked and turned out slower due to a large skew in the number of rows in the presorted groups. An additional quirk with the path generation code meant that we could consider both a sort and an incremental sort on paths with presorted keys. This meant that with the pessimism factor, it was possible that we opted to perform a sort rather than an incremental sort when the given path had presorted keys. Here we remove the 1.5x pessimism factor to allow incremental sorts to have a fairer chance at being chosen against a full sort. Additionally, this changes how incremental sort paths are generated. Previously we would generally create a sort path on the cheapest input path (if that wasn't sorted already) and incremental sort paths on any path which had presorted keys. This meant that if the cheapest input path wasn't completely sorted but happened to have presorted keys, we would create a full sort path *and* an incremental sort path on that input path. Here we change this logic so that if there are presorted keys, we only create an incremental sort path, and create sort paths only when a full sort is required or enable_incremental_sort is disabled. Both the removal of the cost pessimism factor and the changes made to the path generation make it more likely that incremental sorts will now be chosen. That, of course, as with teaching the planner any new tricks, means an increased likelihood that the planner will perform an incremental sort when it's not the best method. Our standard escape hatch for these cases is an enable_* GUC. enable_incremental_sort already exists for this. This came out of a report by Pavel Luzanov where he mentioned that v16 was choosing to perform a Seq Scan -> Sort -> Group Aggregate for his query with an ORDER BY aggregate function. The v15 plan for his query performed an Index Scan -> Group Aggregate, of course, the aggregate performed the final sort internally in nodeAgg.c for the aggregate's ORDER BY. The ideal plan would have been to use the index, which provided partially sorted input then use an incremental sort to provide the aggregate with the sorted input. This was not being chosen due to the pessimism in the incremental sort cost modal, so here we remove that and rationalize the path generation so that sort and incremental sort plans don't have to needlessly complete. Reported-by: Pavel Luzanov Reviewed-by: Richard Guo Discussion: https://postgr.es/m/9f61ddbf-2989-1536-b31e-6459370a6baa%40postgrespro.ru --- .../postgres_fdw/expected/postgres_fdw.out | 2 + contrib/postgres_fdw/sql/postgres_fdw.sql | 2 + src/backend/optimizer/path/allpaths.c | 94 ++-- src/backend/optimizer/path/costsize.c | 38 +- src/backend/optimizer/plan/planner.c | 480 ++++++------------ .../regress/expected/incremental_sort.out | 13 - src/test/regress/expected/partition_join.out | 5 +- src/test/regress/sql/incremental_sort.sql | 5 - 8 files changed, 220 insertions(+), 419 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2ab3f1efaa..5f6e1f9833 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -2796,6 +2796,7 @@ select c2/2, sum(c2) * (c2/2) from ft1 group by c2/2 order by c2/2; (5 rows) -- Aggregates in subquery are pushed down. +set enable_incremental_sort = off; explain (verbose, costs off) select count(x.a), sum(x.a) from (select c2 a, sum(c1) b from ft1 group by c2, sqrt(c1) order by 1, 2) x; QUERY PLAN @@ -2814,6 +2815,7 @@ select count(x.a), sum(x.a) from (select c2 a, sum(c1) b from ft1 group by c2, s 1000 | 4500 (1 row) +reset enable_incremental_sort; -- Aggregate is still pushed down by taking unshippable expression out explain (verbose, costs off) select c2 * (random() <= 1)::int as sum1, sum(c1) * c2 as sum2 from ft1 group by c2 order by 1, 2; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 51560429e0..bfcac4ea14 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -750,9 +750,11 @@ select c2/2, sum(c2) * (c2/2) from ft1 group by c2/2 order by c2/2; select c2/2, sum(c2) * (c2/2) from ft1 group by c2/2 order by c2/2; -- Aggregates in subquery are pushed down. +set enable_incremental_sort = off; explain (verbose, costs off) select count(x.a), sum(x.a) from (select c2 a, sum(c1) b from ft1 group by c2, sqrt(c1) order by 1, 2) x; select count(x.a), sum(x.a) from (select c2 a, sum(c1) b from ft1 group by c2, sqrt(c1) order by 1, 2) x; +reset enable_incremental_sort; -- Aggregate is still pushed down by taking unshippable expression out explain (verbose, costs off) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 0c564be7f6..0cfa3a1d6c 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -3207,70 +3207,52 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r continue; /* - * Consider regular sort for the cheapest partial path (for each - * useful pathkeys). We know the path is not sorted, because we'd - * not get here otherwise. + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially sorted + * already (no need to deal with paths which have presorted keys + * when incremental sort is disabled unless it's the cheapest + * input path). + */ + if (subpath != cheapest_partial_path && + (presorted_keys == 0 || !enable_incremental_sort)) + continue; + + /* + * Consider regular sort for any path that's not presorted or if + * incremental sort is disabled. We've no need to consider both + * sort and incremental sort on the same path. We assume that + * incremental sort is always faster when there are presorted + * keys. * * This is not redundant with the gather paths created in * generate_gather_paths, because that doesn't generate ordered * output. Here we add an explicit sort to match the useful * ordering. */ - if (cheapest_partial_path == subpath) + if (presorted_keys == 0 || !enable_incremental_sort) { - Path *tmp; - - tmp = (Path *) create_sort_path(root, - rel, - subpath, - useful_pathkeys, - -1.0); - - rows = tmp->rows * tmp->parallel_workers; - - path = create_gather_merge_path(root, rel, - tmp, - rel->reltarget, - tmp->pathkeys, - NULL, - rowsp); - - add_path(rel, &path->path); - - /* Fall through */ - } - - /* - * Consider incremental sort, but only when the subpath is already - * partially sorted on a pathkey prefix. - */ - if (enable_incremental_sort && presorted_keys > 0) - { - Path *tmp; - - /* - * We should have already excluded pathkeys of length 1 - * because then presorted_keys > 0 would imply is_sorted was - * true. - */ - Assert(list_length(useful_pathkeys) != 1); - - tmp = (Path *) create_incremental_sort_path(root, - rel, - subpath, - useful_pathkeys, - presorted_keys, - -1); - - path = create_gather_merge_path(root, rel, - tmp, - rel->reltarget, - tmp->pathkeys, - NULL, - rowsp); - - add_path(rel, &path->path); + subpath = (Path *) create_sort_path(root, + rel, + subpath, + useful_pathkeys, + -1.0); + rows = subpath->rows * subpath->parallel_workers; } + else + subpath = (Path *) create_incremental_sort_path(root, + rel, + subpath, + useful_pathkeys, + presorted_keys, + -1); + path = create_gather_merge_path(root, rel, + subpath, + rel->reltarget, + subpath->pathkeys, + NULL, + rowsp); + + add_path(rel, &path->path); } } } diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 897309d7ec..0f0bcfb7e5 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -1959,8 +1959,8 @@ cost_incremental_sort(Path *path, double input_tuples, int width, Cost comparison_cost, int sort_mem, double limit_tuples) { - Cost startup_cost = 0, - run_cost = 0, + Cost startup_cost, + run_cost, input_run_cost = input_total_cost - input_startup_cost; double group_tuples, input_groups; @@ -1969,10 +1969,9 @@ cost_incremental_sort(Path *path, group_input_run_cost; List *presortedExprs = NIL; ListCell *l; - int i = 0; bool unknown_varno = false; - Assert(presorted_keys != 0); + Assert(presorted_keys > 0 && presorted_keys < list_length(pathkeys)); /* * We want to be sure the cost of a sort is never estimated as zero, even @@ -2025,12 +2024,11 @@ cost_incremental_sort(Path *path, /* expression not containing any Vars with "varno 0" */ presortedExprs = lappend(presortedExprs, member->em_expr); - i++; - if (i >= presorted_keys) + if (foreach_current_index(l) + 1 >= presorted_keys) break; } - /* Estimate number of groups with equal presorted keys. */ + /* Estimate the number of groups with equal presorted keys. */ if (!unknown_varno) input_groups = estimate_num_groups(root, presortedExprs, input_tuples, NULL, NULL); @@ -2039,22 +2037,19 @@ cost_incremental_sort(Path *path, group_input_run_cost = input_run_cost / input_groups; /* - * Estimate average cost of sorting of one group where presorted keys are - * equal. Incremental sort is sensitive to distribution of tuples to the - * groups, where we're relying on quite rough assumptions. Thus, we're - * pessimistic about incremental sort performance and increase its average - * group size by half. + * Estimate the average cost of sorting of one group where presorted keys + * are equal. */ cost_tuplesort(&group_startup_cost, &group_run_cost, - 1.5 * group_tuples, width, comparison_cost, sort_mem, + group_tuples, width, comparison_cost, sort_mem, limit_tuples); /* * Startup cost of incremental sort is the startup cost of its first group * plus the cost of its input. */ - startup_cost += group_startup_cost - + input_startup_cost + group_input_run_cost; + startup_cost = group_startup_cost + input_startup_cost + + group_input_run_cost; /* * After we started producing tuples from the first group, the cost of @@ -2062,17 +2057,20 @@ cost_incremental_sort(Path *path, * group, plus the total cost to process the remaining groups, plus the * remaining cost of input. */ - run_cost += group_run_cost - + (group_run_cost + group_startup_cost) * (input_groups - 1) - + group_input_run_cost * (input_groups - 1); + run_cost = group_run_cost + (group_run_cost + group_startup_cost) * + (input_groups - 1) + group_input_run_cost * (input_groups - 1); /* * Incremental sort adds some overhead by itself. Firstly, it has to * detect the sort groups. This is roughly equal to one extra copy and - * comparison per tuple. Secondly, it has to reset the tuplesort context - * for every group. + * comparison per tuple. */ run_cost += (cpu_tuple_cost + comparison_cost) * input_tuples; + + /* + * Additionally, we charge double cpu_tuple_cost for each input group to + * account for the tuplesort_reset that's performed after each group. + */ run_cost += 2.0 * cpu_tuple_cost * input_groups; path->rows = input_tuples; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 5dd4f92720..dfda251d95 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4965,7 +4965,7 @@ create_ordered_paths(PlannerInfo *root, foreach(lc, input_rel->pathlist) { Path *input_path = (Path *) lfirst(lc); - Path *sorted_path = input_path; + Path *sorted_path; bool is_sorted; int presorted_keys; @@ -4973,69 +4973,46 @@ create_ordered_paths(PlannerInfo *root, input_path->pathkeys, &presorted_keys); if (is_sorted) - { - /* Use the input path as is, but add a projection step if needed */ - if (sorted_path->pathtarget != target) - sorted_path = apply_projection_to_path(root, ordered_rel, - sorted_path, target); - - add_path(ordered_rel, sorted_path); - } + sorted_path = input_path; else { /* - * Try adding an explicit sort, but only to the cheapest total - * path since a full sort should generally add the same cost to - * all paths. + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially sorted + * already (no need to deal with paths which have presorted keys + * when incremental sort is disabled unless it's the cheapest + * input path). */ - if (input_path == cheapest_input_path) - { - /* - * Sort the cheapest input path. An explicit sort here can - * take advantage of LIMIT. - */ + if (input_path != cheapest_input_path && + (presorted_keys == 0 || !enable_incremental_sort)) + continue; + + /* + * We've no need to consider both a sort and incremental sort. + * We'll just do a sort if there are no presorted keys and an + * incremental sort when there are presorted keys. + */ + if (presorted_keys == 0 || !enable_incremental_sort) sorted_path = (Path *) create_sort_path(root, ordered_rel, input_path, root->sort_pathkeys, limit_tuples); - /* Add projection step if needed */ - if (sorted_path->pathtarget != target) - sorted_path = apply_projection_to_path(root, ordered_rel, - sorted_path, target); - - add_path(ordered_rel, sorted_path); - } - - /* - * If incremental sort is enabled, then try it as well. Unlike - * with regular sorts, we can't just look at the cheapest path, - * because the cost of incremental sort depends on how well - * presorted the path is. Additionally incremental sort may enable - * a cheaper startup path to win out despite higher total cost. - */ - if (!enable_incremental_sort) - continue; - - /* Likewise, if the path can't be used for incremental sort. */ - if (!presorted_keys) - continue; - - /* Also consider incremental sort. */ - sorted_path = (Path *) create_incremental_sort_path(root, - ordered_rel, - input_path, - root->sort_pathkeys, - presorted_keys, - limit_tuples); + else + sorted_path = (Path *) create_incremental_sort_path(root, + ordered_rel, + input_path, + root->sort_pathkeys, + presorted_keys, + limit_tuples); + } - /* Add projection step if needed */ - if (sorted_path->pathtarget != target) - sorted_path = apply_projection_to_path(root, ordered_rel, - sorted_path, target); + /* Add projection step if needed */ + if (sorted_path->pathtarget != target) + sorted_path = apply_projection_to_path(root, ordered_rel, + sorted_path, target); - add_path(ordered_rel, sorted_path); - } + add_path(ordered_rel, sorted_path); } /* @@ -6501,12 +6478,12 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, { /* * Use any available suitably-sorted path as input, and also consider - * sorting the cheapest-total path. + * sorting the cheapest-total path and incremental sort on any paths + * with presorted keys. */ foreach(lc, input_rel->pathlist) { Path *path = (Path *) lfirst(lc); - Path *path_original = path; bool is_sorted; int presorted_keys; @@ -6514,90 +6491,39 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, path->pathkeys, &presorted_keys); - if (path == cheapest_path || is_sorted) + if (!is_sorted) { - /* Sort the cheapest-total path if it isn't already sorted */ - if (!is_sorted) + /* + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially sorted + * already (no need to deal with paths which have presorted + * keys when incremental sort is disabled unless it's the + * cheapest input path). + */ + if (path != cheapest_path && + (presorted_keys == 0 || !enable_incremental_sort)) + continue; + + /* + * We've no need to consider both a sort and incremental sort. + * We'll just do a sort if there are no presorted keys and an + * incremental sort when there are presorted keys. + */ + if (presorted_keys == 0 || !enable_incremental_sort) path = (Path *) create_sort_path(root, grouped_rel, path, root->group_pathkeys, -1.0); - - /* Now decide what to stick atop it */ - if (parse->groupingSets) - { - consider_groupingsets_paths(root, grouped_rel, - path, true, can_hash, - gd, agg_costs, dNumGroups); - } - else if (parse->hasAggs) - { - /* - * We have aggregation, possibly with plain GROUP BY. Make - * an AggPath. - */ - add_path(grouped_rel, (Path *) - create_agg_path(root, - grouped_rel, - path, - grouped_rel->reltarget, - parse->groupClause ? AGG_SORTED : AGG_PLAIN, - AGGSPLIT_SIMPLE, - parse->groupClause, - havingQual, - agg_costs, - dNumGroups)); - } - else if (parse->groupClause) - { - /* - * We have GROUP BY without aggregation or grouping sets. - * Make a GroupPath. - */ - add_path(grouped_rel, (Path *) - create_group_path(root, - grouped_rel, - path, - parse->groupClause, - havingQual, - dNumGroups)); - } else - { - /* Other cases should have been handled above */ - Assert(false); - } + path = (Path *) create_incremental_sort_path(root, + grouped_rel, + path, + root->group_pathkeys, + presorted_keys, + -1.0); } - /* - * Now we may consider incremental sort on this path, but only - * when the path is not already sorted and when incremental sort - * is enabled. - */ - if (is_sorted || !enable_incremental_sort) - continue; - - /* Restore the input path (we might have added Sort on top). */ - path = path_original; - - /* no shared prefix, no point in building incremental sort */ - if (presorted_keys == 0) - continue; - - /* - * We should have already excluded pathkeys of length 1 because - * then presorted_keys > 0 would imply is_sorted was true. - */ - Assert(list_length(root->group_pathkeys) != 1); - - path = (Path *) create_incremental_sort_path(root, - grouped_rel, - path, - root->group_pathkeys, - presorted_keys, - -1.0); - /* Now decide what to stick atop it */ if (parse->groupingSets) { @@ -6653,7 +6579,6 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, foreach(lc, partially_grouped_rel->pathlist) { Path *path = (Path *) lfirst(lc); - Path *path_original = path; bool is_sorted; int presorted_keys; @@ -6661,19 +6586,38 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, path->pathkeys, &presorted_keys); - /* - * Insert a Sort node, if required. But there's no point in - * sorting anything but the cheapest path. - */ if (!is_sorted) { - if (path != partially_grouped_rel->cheapest_total_path) + /* + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially + * sorted already (no need to deal with paths which have + * presorted keys when incremental sort is disabled unless + * it's the cheapest input path). + */ + if (path != partially_grouped_rel->cheapest_total_path && + (presorted_keys == 0 || !enable_incremental_sort)) continue; - path = (Path *) create_sort_path(root, - grouped_rel, - path, - root->group_pathkeys, - -1.0); + + /* + * We've no need to consider both a sort and incremental + * sort. We'll just do a sort if there are no pre-sorted + * keys and an incremental sort when there are presorted + * keys. + */ + if (presorted_keys == 0 || !enable_incremental_sort) + path = (Path *) create_sort_path(root, + grouped_rel, + path, + root->group_pathkeys, + -1.0); + else + path = (Path *) create_incremental_sort_path(root, + grouped_rel, + path, + root->group_pathkeys, + presorted_keys, + -1.0); } if (parse->hasAggs) @@ -6697,55 +6641,6 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, havingQual, dNumGroups)); - /* - * Now we may consider incremental sort on this path, but only - * when the path is not already sorted and when incremental - * sort is enabled. - */ - if (is_sorted || !enable_incremental_sort) - continue; - - /* Restore the input path (we might have added Sort on top). */ - path = path_original; - - /* no shared prefix, not point in building incremental sort */ - if (presorted_keys == 0) - continue; - - /* - * We should have already excluded pathkeys of length 1 - * because then presorted_keys > 0 would imply is_sorted was - * true. - */ - Assert(list_length(root->group_pathkeys) != 1); - - path = (Path *) create_incremental_sort_path(root, - grouped_rel, - path, - root->group_pathkeys, - presorted_keys, - -1.0); - - if (parse->hasAggs) - add_path(grouped_rel, (Path *) - create_agg_path(root, - grouped_rel, - path, - grouped_rel->reltarget, - parse->groupClause ? AGG_SORTED : AGG_PLAIN, - AGGSPLIT_FINAL_DESERIAL, - parse->groupClause, - havingQual, - agg_final_costs, - dNumGroups)); - else - add_path(grouped_rel, (Path *) - create_group_path(root, - grouped_rel, - path, - parse->groupClause, - havingQual, - dNumGroups)); } } } @@ -6950,97 +6845,64 @@ create_partial_grouping_paths(PlannerInfo *root, { Path *path = (Path *) lfirst(lc); bool is_sorted; + int presorted_keys; - is_sorted = pathkeys_contained_in(root->group_pathkeys, - path->pathkeys); - if (path == cheapest_total_path || is_sorted) + is_sorted = pathkeys_count_contained_in(root->group_pathkeys, + path->pathkeys, + &presorted_keys); + if (!is_sorted) { - /* Sort the cheapest partial path, if it isn't already */ - if (!is_sorted) + /* + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially sorted + * already (no need to deal with paths which have presorted + * keys when incremental sort is disabled unless it's the + * cheapest input path). + */ + if (path != cheapest_total_path && + (presorted_keys == 0 || !enable_incremental_sort)) + continue; + + /* + * We've no need to consider both a sort and incremental sort. + * We'll just do a sort if there are no presorted keys and an + * incremental sort when there are presorted keys. + */ + if (presorted_keys == 0 || !enable_incremental_sort) path = (Path *) create_sort_path(root, partially_grouped_rel, path, root->group_pathkeys, -1.0); - - if (parse->hasAggs) - add_path(partially_grouped_rel, (Path *) - create_agg_path(root, - partially_grouped_rel, - path, - partially_grouped_rel->reltarget, - parse->groupClause ? AGG_SORTED : AGG_PLAIN, - AGGSPLIT_INITIAL_SERIAL, - parse->groupClause, - NIL, - agg_partial_costs, - dNumPartialGroups)); else - add_path(partially_grouped_rel, (Path *) - create_group_path(root, - partially_grouped_rel, - path, - parse->groupClause, - NIL, - dNumPartialGroups)); + path = (Path *) create_incremental_sort_path(root, + partially_grouped_rel, + path, + root->group_pathkeys, + presorted_keys, + -1.0); } - } - - /* - * Consider incremental sort on all partial paths, if enabled. - * - * We can also skip the entire loop when we only have a single-item - * group_pathkeys because then we can't possibly have a presorted - * prefix of the list without having the list be fully sorted. - */ - if (enable_incremental_sort && list_length(root->group_pathkeys) > 1) - { - foreach(lc, input_rel->pathlist) - { - Path *path = (Path *) lfirst(lc); - bool is_sorted; - int presorted_keys; - - is_sorted = pathkeys_count_contained_in(root->group_pathkeys, - path->pathkeys, - &presorted_keys); - - /* Ignore already sorted paths */ - if (is_sorted) - continue; - - if (presorted_keys == 0) - continue; - /* Since we have presorted keys, consider incremental sort. */ - path = (Path *) create_incremental_sort_path(root, - partially_grouped_rel, - path, - root->group_pathkeys, - presorted_keys, - -1.0); - - if (parse->hasAggs) - add_path(partially_grouped_rel, (Path *) - create_agg_path(root, - partially_grouped_rel, - path, - partially_grouped_rel->reltarget, - parse->groupClause ? AGG_SORTED : AGG_PLAIN, - AGGSPLIT_INITIAL_SERIAL, - parse->groupClause, - NIL, - agg_partial_costs, - dNumPartialGroups)); - else - add_path(partially_grouped_rel, (Path *) - create_group_path(root, - partially_grouped_rel, - path, - parse->groupClause, - NIL, - dNumPartialGroups)); - } + if (parse->hasAggs) + add_path(partially_grouped_rel, (Path *) + create_agg_path(root, + partially_grouped_rel, + path, + partially_grouped_rel->reltarget, + parse->groupClause ? AGG_SORTED : AGG_PLAIN, + AGGSPLIT_INITIAL_SERIAL, + parse->groupClause, + NIL, + agg_partial_costs, + dNumPartialGroups)); + else + add_path(partially_grouped_rel, (Path *) + create_group_path(root, + partially_grouped_rel, + path, + parse->groupClause, + NIL, + dNumPartialGroups)); } } @@ -7050,7 +6912,6 @@ create_partial_grouping_paths(PlannerInfo *root, foreach(lc, input_rel->partial_pathlist) { Path *path = (Path *) lfirst(lc); - Path *path_original = path; bool is_sorted; int presorted_keys; @@ -7058,66 +6919,39 @@ create_partial_grouping_paths(PlannerInfo *root, path->pathkeys, &presorted_keys); - if (path == cheapest_partial_path || is_sorted) + if (!is_sorted) { - /* Sort the cheapest partial path, if it isn't already */ - if (!is_sorted) + /* + * Try at least sorting the cheapest path and also try + * incrementally sorting any path which is partially sorted + * already (no need to deal with paths which have presorted + * keys when incremental sort is disabled unless it's the + * cheapest input path). + */ + if (path != cheapest_partial_path && + (presorted_keys == 0 || !enable_incremental_sort)) + continue; + + /* + * We've no need to consider both a sort and incremental sort. + * We'll just do a sort if there are no presorted keys and an + * incremental sort when there are presorted keys. + */ + if (presorted_keys == 0 || !enable_incremental_sort) path = (Path *) create_sort_path(root, partially_grouped_rel, path, root->group_pathkeys, -1.0); - - if (parse->hasAggs) - add_partial_path(partially_grouped_rel, (Path *) - create_agg_path(root, - partially_grouped_rel, - path, - partially_grouped_rel->reltarget, - parse->groupClause ? AGG_SORTED : AGG_PLAIN, - AGGSPLIT_INITIAL_SERIAL, - parse->groupClause, - NIL, - agg_partial_costs, - dNumPartialPartialGroups)); else - add_partial_path(partially_grouped_rel, (Path *) - create_group_path(root, - partially_grouped_rel, - path, - parse->groupClause, - NIL, - dNumPartialPartialGroups)); + path = (Path *) create_incremental_sort_path(root, + partially_grouped_rel, + path, + root->group_pathkeys, + presorted_keys, + -1.0); } - /* - * Now we may consider incremental sort on this path, but only - * when the path is not already sorted and when incremental sort - * is enabled. - */ - if (is_sorted || !enable_incremental_sort) - continue; - - /* Restore the input path (we might have added Sort on top). */ - path = path_original; - - /* no shared prefix, not point in building incremental sort */ - if (presorted_keys == 0) - continue; - - /* - * We should have already excluded pathkeys of length 1 because - * then presorted_keys > 0 would imply is_sorted was true. - */ - Assert(list_length(root->group_pathkeys) != 1); - - path = (Path *) create_incremental_sort_path(root, - partially_grouped_rel, - path, - root->group_pathkeys, - presorted_keys, - -1.0); - if (parse->hasAggs) add_partial_path(partially_grouped_rel, (Path *) create_agg_path(root, diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out index 0a631124c2..1a1e8b2365 100644 --- a/src/test/regress/expected/incremental_sort.out +++ b/src/test/regress/expected/incremental_sort.out @@ -1,16 +1,3 @@ --- When we have to sort the entire table, incremental sort will --- be slower than plain sort, so it should not be used. -explain (costs off) -select * from (select * from tenk1 order by four) t order by four, ten; - QUERY PLAN ------------------------------------ - Sort - Sort Key: tenk1.four, tenk1.ten - -> Sort - Sort Key: tenk1.four - -> Seq Scan on tenk1 -(5 rows) - -- When there is a LIMIT clause, incremental sort is beneficial because -- it only has to sort some of the groups, and not the entire table. explain (costs off) diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out index b20facc19f..c59caf1cb3 100644 --- a/src/test/regress/expected/partition_join.out +++ b/src/test/regress/expected/partition_join.out @@ -1173,8 +1173,9 @@ EXPLAIN (COSTS OFF) SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b; QUERY PLAN ----------------------------------------------------------- - Sort + Incremental Sort Sort Key: prt1.a, prt2.b + Presorted Key: prt1.a -> Merge Left Join Merge Cond: (prt1.a = prt2.b) -> Sort @@ -1191,7 +1192,7 @@ SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * Filter: (b > 250) -> Seq Scan on prt2_p3 prt2_2 Filter: (b > 250) -(18 rows) +(19 rows) SELECT t1.a, t2.b FROM (SELECT * FROM prt1 WHERE a < 450) t1 LEFT JOIN (SELECT * FROM prt2 WHERE b > 250) t2 ON t1.a = t2.b WHERE t1.b = 0 ORDER BY t1.a, t2.b; a | b diff --git a/src/test/regress/sql/incremental_sort.sql b/src/test/regress/sql/incremental_sort.sql index 284a354dbb..071f8a5268 100644 --- a/src/test/regress/sql/incremental_sort.sql +++ b/src/test/regress/sql/incremental_sort.sql @@ -1,8 +1,3 @@ --- When we have to sort the entire table, incremental sort will --- be slower than plain sort, so it should not be used. -explain (costs off) -select * from (select * from tenk1 order by four) t order by four, ten; - -- When there is a LIMIT clause, incremental sort is beneficial because -- it only has to sort some of the groups, and not the entire table. explain (costs off) -- 2.35.1.windows.2