From 4a84528d96b61b5a4828b8735f5418a79bcf526e Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 19 Mar 2020 01:20:58 +0100 Subject: [PATCH 2/5] fixes --- src/backend/executor/nodeAgg.c | 3 +-- src/backend/optimizer/plan/createplan.c | 15 ++++++++--- src/backend/optimizer/plan/planner.c | 47 ++++++++++++++++++++++++++------- 3 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index bf484e19ec..ebd267db68 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -516,7 +516,7 @@ initialize_phase(AggState *aggstate, int newphase) */ if (newphase > 0 && newphase < aggstate->numphases - 1) { - Sort *sortnode = (Sort *)aggstate->phases[newphase + 1].aggnode->sortnode; + Sort *sortnode = (Sort *) aggstate->phases[newphase + 1].aggnode->sortnode; PlanState *outerNode = outerPlanState(aggstate); TupleDesc tupDesc = ExecGetResultType(outerNode); @@ -4672,7 +4672,6 @@ ExecReScanAgg(AggState *node) node->projected_set = -1; } - if (outerPlan->chgParam == NULL) ExecReScan(outerPlan); } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index d5b34089aa..7c29f89cc3 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2171,10 +2171,9 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path) /* * Agg can project, so no need to be terribly picky about child tlist, but - * we do need grouping columns to be available; If the groupingsets need + * we do need grouping columns to be available. If the groupingsets need * to sort the input, the agg will store the input rows in a tuplesort, - * it therefore behooves us to request a small tlist to avoid wasting - * spaces. + * so we request a small tlist to avoid wasting space. */ if (!best_path->is_sorted) flags = flags | CP_SMALL_TLIST; @@ -2239,6 +2238,11 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path) new_grpColIdx = remap_groupColIdx(root, rollup->groupClause); + /* + * If it's the first rollup using sorted mode, add an explicit sort + * node if the input is not sorted yet, for other rollups using + * sorted mode, always add an explicit sort. + */ if (!rollup->is_hashed) { if (!is_first_sort || @@ -2297,7 +2301,10 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path) top_grpColIdx = remap_groupColIdx(root, rollup->groupClause); - /* the input is not sorted yet */ + /* + * When the rollup uses sorted mode, and the input is not already sorted, + * add an explicit sort. + */ if (!rollup->is_hashed && !best_path->is_sorted) { diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index b7858e8d02..6578b3fef0 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4188,13 +4188,22 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, * times, so it's important that it not scribble on input. No result is * returned, but any generated paths are added to grouped_rel. * - * - strat: - * preferred aggregate strategy to use. - * - * - is_sorted: - * Is the input sorted on the groupCols of the first rollup. Caller - * must set it correctly if strat is set to AGG_SORTED, the planner - * uses it to generate a sortnode. + * The caller specifies the preferred aggregate strategy (sorted or hashed) using + * the strat aprameter. When the requested strategy is AGG_SORTED, the input path + * needs to be sorted accordingly (is_sorted needs to be true). + * + * Pengzhou: is_sorted is acutally a hint here, the callers prefer to use + * AGG_SORTED are not forced to add an explicit sort path before calling + * this function now. please see comments in callers + * + * Ideally, consider_groupingsets_paths() should check whether the input is + * sorted or not, however, the callers prefer using AGG_SORTED is forced to + * check is_sorted already (to see whether a non-cheapest-path is worth + * considering), so consider_groupingsets_paths() don't need to check it again. + * for callers prefer AGG_HASHED, is_sorted is never checked, they only consider + * the cheapest path, but the cheapest path can also be already sorted + * coincidentally, that's why AGG_MIZED is choosen when strat is specified + * to AGG_HASHED. */ static void consider_groupingsets_paths(PlannerInfo *root, @@ -4262,7 +4271,7 @@ consider_groupingsets_paths(PlannerInfo *root, unhashed_rollup = lfirst_node(RollupData, l_start); exclude_groups = unhashed_rollup->numGroups; l_start = lnext(gd->rollups, l_start); - /* update is_sorted to true */ + /* the input is coincidentally sorted usefully, update is_sorted */ is_sorted = true; } @@ -4362,7 +4371,10 @@ consider_groupingsets_paths(PlannerInfo *root, rollup->hashable = false; rollup->is_hashed = false; new_rollups = lappend(new_rollups, rollup); - /* update is_sorted to true */ + /* + * The first non-hashed rollup is PLAIN AGG, is_sorted + * should be true. + */ is_sorted = true; strat = AGG_MIXED; } @@ -4397,6 +4409,9 @@ consider_groupingsets_paths(PlannerInfo *root, * * can_hash is passed in as false if some obstacle elsewhere (such as * ordered aggs) means that we shouldn't consider hashing at all. + * + * XXX This comment seems to be broken by the patch, and it's not very + * clear to me what it tries to say. */ if (can_hash && gd->any_hashable) { @@ -4448,7 +4463,7 @@ consider_groupingsets_paths(PlannerInfo *root, /* * We leave the first rollup out of consideration since it's the - * one that need to be sorted. We assign indexes "i" + * one that matches the input sort order. We assign indexes "i" * to only those entries considered for hashing; the second loop, * below, must use the same condition. */ @@ -6422,6 +6437,18 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, path->pathkeys); if (path == cheapest_path || is_sorted) { + /* XXX Why do we do it before possibly adding an explicit sort on top? */ + /* + * Pengzhou: this patch intend to let each sorted aggregate phases + * do their own sorting include the first phase, so in the final + * stage of parallel grouping sets, the tuples is put into temp + * storage of each sorted phase and then each sorted phase do + * its own sorting one by one. + * Add a explicit sort path underneath the main Agg node will + * make tuples from all groupingsets sorted using the sort key + * of the first phase, it is not right. + * + */ if (parse->groupingSets) { /* consider AGG_SORTED strategy */ -- 2.14.1