From 57f30d60158398bd57f02ef00800d3f63c6ab12f Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 19 Mar 2020 01:20:58 +0100 Subject: [PATCH 2/7] fixes --- src/backend/executor/nodeAgg.c | 3 +-- src/backend/optimizer/plan/createplan.c | 14 ++++++++++---- src/backend/optimizer/plan/planner.c | 23 +++++++++++++---------- src/test/modules/unsafe_tests/Makefile | 2 +- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index b4f53bf77a..c3a043e448 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -368,7 +368,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); @@ -3620,7 +3620,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..044ec92aa8 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,10 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path) new_grpColIdx = remap_groupColIdx(root, rollup->groupClause); + /* + * FIXME This combination of nested if checks needs some explanation + * why we need this particular combination of flags. + */ if (!rollup->is_hashed) { if (!is_first_sort || @@ -2297,7 +2300,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 82a15761b4..fc6a1d0044 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -4188,13 +4188,9 @@ 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). */ static void consider_groupingsets_paths(PlannerInfo *root, @@ -4262,7 +4258,8 @@ 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 */ + /* update is_sorted to true + * XXX why? shouldn't it be already set by the caller? */ is_sorted = true; } @@ -4361,7 +4358,9 @@ consider_groupingsets_paths(PlannerInfo *root, rollup->hashable = false; rollup->is_hashed = false; new_rollups = lappend(new_rollups, rollup); - /* update is_sorted to true */ + /* update is_sorted to true + * XXX why? shouldn't it be already set by the caller? + */ is_sorted = true; strat = AGG_MIXED; } @@ -4396,6 +4395,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) { @@ -4447,7 +4449,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. */ @@ -6421,6 +6423,7 @@ 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? */ if (parse->groupingSets) { /* consider AGG_SORTED strategy */ diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile index 3ecf5fcfc5..2cf710eb2c 100644 --- a/src/test/modules/unsafe_tests/Makefile +++ b/src/test/modules/unsafe_tests/Makefile @@ -1,6 +1,6 @@ # src/test/modules/unsafe_tests/Makefile -REGRESS = rolenames alter_system_table +REGRESS = alter_system_table ifdef USE_PGXS PG_CONFIG = pg_config -- 2.21.1