From 2b305be1a9b7423075f02b865ddbf10461407061 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Mon, 17 Jul 2023 14:08:10 +0800 Subject: [PATCH v2 2/2] Revise how we sort partial paths in gather_grouping_paths --- src/backend/optimizer/plan/planner.c | 49 ++----------------- src/test/regress/expected/select_parallel.out | 16 ++++++ src/test/regress/sql/select_parallel.sql | 4 ++ 3 files changed, 24 insertions(+), 45 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index cf0a3fad2a..065a4f0f07 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -7380,7 +7380,6 @@ create_partial_grouping_paths(PlannerInfo *root, static void gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel) { - ListCell *lc; Path *cheapest_partial_path; /* Try Gather for unordered paths and Gather Merge for ordered ones. */ @@ -7412,51 +7411,11 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel) } /* - * 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. + * We do not need to consider incremental sort on partial paths, because + * for a partial path of a grouped or partially grouped relation, it is + * either unordered (HashAggregate or Append), or it has been ordered by + * the group_pathkeys (GroupAggregate). */ - if (!enable_incremental_sort || list_length(root->group_pathkeys) == 1) - return; - - /* also consider incremental sort on partial paths, if enabled */ - foreach(lc, rel->partial_pathlist) - { - Path *path = (Path *) lfirst(lc); - bool is_sorted; - int presorted_keys; - double total_groups; - - is_sorted = pathkeys_count_contained_in(root->group_pathkeys, - path->pathkeys, - &presorted_keys); - - if (is_sorted) - continue; - - if (presorted_keys == 0) - continue; - - path = (Path *) create_incremental_sort_path(root, - rel, - path, - root->group_pathkeys, - presorted_keys, - -1.0); - - path = (Path *) - create_gather_merge_path(root, - rel, - path, - rel->reltarget, - root->group_pathkeys, - NULL, - &total_groups); - - add_path(rel, path); - } } /* diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out index d82a7799e6..5d1b8e05c9 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -970,6 +970,22 @@ select * from tenk1 where four = 2 order by four, hundred, parallel_safe_volatil reset min_parallel_index_scan_size; reset enable_seqscan; +-- gather merge atop sort of grouped rel's partial paths +explain (costs off) +select count(*) from tenk1 group by twenty, parallel_safe_volatile(two); + QUERY PLAN +-------------------------------------------------------------------- + Finalize GroupAggregate + Group Key: twenty, (parallel_safe_volatile(two)) + -> Gather Merge + Workers Planned: 4 + -> Sort + Sort Key: twenty, (parallel_safe_volatile(two)) + -> Partial HashAggregate + Group Key: twenty, parallel_safe_volatile(two) + -> Parallel Seq Scan on tenk1 +(9 rows) + SAVEPOINT settings; SET LOCAL debug_parallel_query = 1; explain (costs off) diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql index 7cba6519cb..0f9e874ae9 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -360,6 +360,10 @@ select * from tenk1 where four = 2 order by four, hundred, parallel_safe_volatil reset min_parallel_index_scan_size; reset enable_seqscan; +-- gather merge atop sort of grouped rel's partial paths +explain (costs off) +select count(*) from tenk1 group by twenty, parallel_safe_volatile(two); + SAVEPOINT settings; SET LOCAL debug_parallel_query = 1; explain (costs off) -- 2.31.0