From 753184e345638b0c681ba6ba1d4b3a2bf7b4c570 Mon Sep 17 00:00:00 2001 From: James Coleman Date: Wed, 1 Apr 2020 08:53:18 -0400 Subject: [PATCH v50 6/6] cleanup --- src/backend/optimizer/path/allpaths.c | 13 ++++- src/backend/optimizer/plan/planner.c | 81 +++++++++++++++++---------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 006924d4a6..4a72dcf8bc 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2842,7 +2842,10 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r Path *subpath = (Path *) lfirst(lc2); GatherMergePath *path; - /* path has no ordering at all, can't use incremental sort */ + /* + * If the path has no ordering at all, then we can't use either + * incremental sort or rely on implict sorting with a gather merge. + */ if (subpath->pathkeys == NIL) continue; @@ -2867,8 +2870,6 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r continue; } - Assert(!is_sorted); - /* * Consider regular sort for the cheapest partial path (for each * useful pathkeys). We know the path is not sorted, because we'd @@ -2911,6 +2912,12 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r { 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, diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 1cfbd88eec..9608fdaec8 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -5096,8 +5096,12 @@ create_ordered_paths(PlannerInfo *root, * * XXX This is probably duplicate with the paths we already generate * in generate_useful_gather_paths in apply_scanjoin_target_to_paths. + * + * We can also skip the entire loop when we only have a single-item + * sort_pathkeys because then we can't possibly have a presorted + * prefix of the list without having the list be fully sorted. */ - if (enable_incrementalsort) + if (enable_incrementalsort && list_length(root->sort_pathkeys) > 1) { ListCell *lc; @@ -6509,8 +6513,10 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, bool is_sorted; int presorted_keys; - is_sorted = pathkeys_contained_in(root->group_pathkeys, - path->pathkeys); + is_sorted = pathkeys_count_contained_in(root->group_pathkeys, + path->pathkeys, + &presorted_keys); + if (path == cheapest_path || is_sorted) { /* Sort the cheapest-total path if it isn't already sorted */ @@ -6578,17 +6584,16 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, /* Restore the input path (we might have added Sort on top). */ path = path_original; - is_sorted = pathkeys_count_contained_in(root->group_pathkeys, - path->pathkeys, - &presorted_keys); - - /* We've already skipped fully sorted paths above. */ - Assert(!is_sorted); - /* 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, @@ -6655,8 +6660,9 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, bool is_sorted; int presorted_keys; - is_sorted = pathkeys_contained_in(root->group_pathkeys, - path->pathkeys); + is_sorted = pathkeys_count_contained_in(root->group_pathkeys, + path->pathkeys, + &presorted_keys); /* * Insert a Sort node, if required. But there's no point in @@ -6705,17 +6711,16 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, /* Restore the input path (we might have added Sort on top). */ path = path_original; - is_sorted = pathkeys_count_contained_in(root->group_pathkeys, - path->pathkeys, - &presorted_keys); - - /* We've already skipped fully sorted paths above. */ - Assert(!is_sorted); - /* 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, @@ -7015,8 +7020,14 @@ create_partial_grouping_paths(PlannerInfo *root, } } - /* Consider incremental sort on all partial paths, if enabled. */ - if (enable_incrementalsort) + /* + * 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_incrementalsort && list_length(root->group_pathkeys) > 1) { foreach(lc, input_rel->pathlist) { @@ -7078,8 +7089,10 @@ create_partial_grouping_paths(PlannerInfo *root, bool is_sorted; int presorted_keys; - is_sorted = pathkeys_contained_in(root->group_pathkeys, - path->pathkeys); + is_sorted = pathkeys_count_contained_in(root->group_pathkeys, + path->pathkeys, + &presorted_keys); + if (path == cheapest_partial_path || is_sorted) { /* Sort the cheapest partial path, if it isn't already */ @@ -7123,17 +7136,16 @@ create_partial_grouping_paths(PlannerInfo *root, /* Restore the input path (we might have added Sort on top). */ path = path_original; - is_sorted = pathkeys_count_contained_in(root->group_pathkeys, - path->pathkeys, - &presorted_keys); - - /* We've already skipped fully sorted paths above. */ - Assert(!is_sorted); - /* 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, @@ -7289,7 +7301,14 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel) add_path(rel, path); } - if (!enable_incrementalsort) + /* + * 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_incrementalsort || list_length(root->group_pathkeys) == 1) return; /* also consider incremental sort on partial paths, if enabled */ -- 2.17.1