From d0ee7837d2e8137390f6e64188ecbd4a1addb11d Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 19 Mar 2020 16:55:28 +0100 Subject: [PATCH 4/8] fix --- src/backend/commands/explain.c | 6 ++-- src/backend/executor/execProcnode.c | 11 ++++---- src/backend/optimizer/plan/planner.c | 41 +++++++++++++++++----------- src/backend/utils/sort/tuplesort.c | 10 +++++-- src/include/nodes/plannodes.h | 1 - 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index dd4600a214..0256dd42f1 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -2767,7 +2767,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo, foreach(methodCell, groupInfo->sortMethods) { - const *sortMethodName = tuplesort_method_name(methodCell->int_value); + const char *sortMethodName = tuplesort_method_name(methodCell->int_value); methodNames = lappend(methodNames, sortMethodName); } @@ -2776,7 +2776,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo, if (groupInfo->maxMemorySpaceUsed > 0) { long avgSpace = groupInfo->totalMemorySpaceUsed / groupInfo->groupCount; - const *spaceTypeName; + const char *spaceTypeName; ExplainPropertyInteger("Average Sort Space Used", "kB", avgSpace, es); ExplainPropertyInteger("Maximum Sort Space Used", "kB", @@ -2787,7 +2787,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo, if (groupInfo->maxDiskSpaceUsed > 0) { long avgSpace = groupInfo->totalDiskSpaceUsed / groupInfo->groupCount; - const *spaceTypeName; + const char *spaceTypeName; ExplainPropertyInteger("Average Sort Space Used", "kB", avgSpace, es); ExplainPropertyInteger("Maximum Sort Space Used", "kB", diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c index d15a86a706..5662e7d742 100644 --- a/src/backend/executor/execProcnode.c +++ b/src/backend/executor/execProcnode.c @@ -852,12 +852,13 @@ ExecSetTupleBound(int64 tuples_needed, PlanState *child_node) else if (IsA(child_node, IncrementalSortState)) { /* - * If it is a Sort node, notify it that it can use bounded sort. + * If it is an IncrementalSort node, notify it that it can use bounded + * sort. * - * Note: it is the responsibility of nodeSort.c to react properly to - * changes of these parameters. If we ever redesign this, it'd be a - * good idea to integrate this signaling with the parameter-change - * mechanism. + * Note: it is the responsibility of nodeIncrementalSort.c to react + * properly to changes of these parameters. If we ever redesign this, + * it'd be a good idea to integrate this signaling with the + * parameter-change mechanism. */ IncrementalSortState *sortState = (IncrementalSortState *) child_node; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index b2f4aaadb5..0e01cf8cb1 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4869,7 +4869,7 @@ create_distinct_paths(PlannerInfo *root, else { Size hashentrysize = hash_agg_entry_size( - 0, cheapest_input_path->pathtarget->width, 0); + 0, cheapest_input_path->pathtarget->width, 0); /* Allow hashing only if hashtable is predicted to fit in work_mem */ allow_hash = (hashentrysize * numDistinctRows <= work_mem * 1024L); @@ -4932,6 +4932,9 @@ create_distinct_paths(PlannerInfo *root, * target: the output tlist the result Paths must emit * limit_tuples: estimated bound on the number of output tuples, * or -1 if no LIMIT or couldn't estimate + * + * XXX This only looks at sort_pathkeys. I wonder if it needs to look at the + * other pathkeys (grouping, ...) like generate_useful_gather_paths. */ static RelOptInfo * create_ordered_paths(PlannerInfo *root, @@ -5002,23 +5005,29 @@ create_ordered_paths(PlannerInfo *root, add_path(ordered_rel, sorted_path); } - if (enable_incrementalsort && presorted_keys > 0) - { - /* Also consider incremental sort. */ - 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); + /* With incremental sort disabled, don't build those paths. */ + if (!enable_incrementalsort) + continue; - add_path(ordered_rel, sorted_path); - } + /* 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); + + /* 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); } } diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 583551d197..77c15ebd78 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -256,8 +256,8 @@ struct Tuplesortstate bool isMaxSpaceDisk; /* true when maxSpace is value for on-disk * space, false when it's value for in-memory * space */ - TupSortStatus maxSpaceStatus; /* sort status when maxSpace was reached */ - MemoryContext maincontext; /* memory context for tuple sort metadata that + TupSortStatus maxSpaceStatus; /* sort status when maxSpace was reached */ + MemoryContext maincontext; /* memory context for tuple sort metadata that * persists across multiple batches */ MemoryContext sortcontext; /* memory context holding most sort data */ MemoryContext tuplecontext; /* sub-context of sortcontext for tuple data */ @@ -803,6 +803,9 @@ tuplesort_begin_common(int workMem, SortCoordinate coordinate, return state; } +/* + * XXX Missing comment. + */ static void tuplesort_begin_batch(Tuplesortstate *state) { @@ -1288,6 +1291,9 @@ tuplesort_set_bound(Tuplesortstate *state, int64 bound) state->sortKeys->abbrev_full_comparator = NULL; } +/* + * XXX Missing comment. + */ bool tuplesort_used_bound(Tuplesortstate *state) { diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index fe4046b64b..136d794219 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -774,7 +774,6 @@ typedef struct Sort bool *nullsFirst; /* NULLS FIRST/LAST directions */ } Sort; - /* ---------------- * incremental sort node * ---------------- -- 2.21.1