Subject: [PATCH 1/7] All grouping sets do their own sorting

From: Pengzhou Tang <[email-masked]>
Date: Wed, 11 Mar 2020 23:07:29 -0400

PG used to add a SORT path explicitly beneath the AGG for sort aggregate,
grouping sets path also add a SORT path for the first sort aggregate phase,
but the following sort aggregate phases do their own sorting using a tuplesort.
This commit unified the way how grouping sets path doing sort, all sort aggregate
phases now do their own sorting using tuplesort.

This commit is mainly a preparing step to support parallel grouping sets, the
main idea of parallel grouping sets is: like parallel aggregate,  we separate
grouping sets into two stages:

The initial stage: this stage has almost the same plan and execution routines
with the current implementation of grouping sets, the differenceis are 1) it
only produces partial aggregate results 2) the output is attached with an extra
grouping set id. We know partial aggregate results will be combined in the final
stage and we have multiple grouping sets, so only partial aggregate results
belong to the same grouping set can be combined, that is why grouping set id is
introduced to identify the sets. We keep all the optimizations of multiple
grouping sets in the initial stage, eg, 1) the grouping sets (that can be
grouped by one single sort) are put into the one rollup structure so those sets
arecomputed in one aggregate phase. 2) do hash aggregate concurrently when a
sort aggregate is performed. 3) do all hash transitions in one expression state.

The final stage: this stage combine the partial aggregate results according to
the grouping set id. Obviously, all the optimizations in the initial stage
cannot be used, so all rollups are extracted, each rollup contains only one
grouping set, then each aggregate phase only processes one set. We do a filter
in the final stage, redirect the tuples to each aggregate phase.

Obviously, adding a SORT path underneath the AGG in the final stage is not
right. This commit can avoid it and all non-hashed aggregate phases can do
their own sorting after thetuples are redirected.
---
 src/backend/commands/explain.c             |   5 +-
 src/backend/executor/nodeAgg.c             |  79 +++++++++++--
 src/backend/nodes/copyfuncs.c              |   1 +
 src/backend/nodes/outfuncs.c               |   1 +
 src/backend/nodes/readfuncs.c              |   1 +
 src/backend/optimizer/plan/createplan.c    |  65 ++++++++---
 src/backend/optimizer/plan/planner.c       |  66 +++++++----
 src/backend/optimizer/util/pathnode.c      |  30 ++++-
 src/include/executor/nodeAgg.h             |   2 -
 src/include/nodes/execnodes.h              |   5 +-
 src/include/nodes/pathnodes.h              |   1 +
 src/include/nodes/plannodes.h              |   1 +
 src/include/optimizer/pathnode.h           |   3 +-
 src/include/optimizer/planmain.h           |   2 +-
 src/test/regress/expected/groupingsets.out | 130 ++++++++++-----------
 15 files changed, 260 insertions(+), 132 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4a50..b1609b339a 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2289,15 +2289,14 @@ show_grouping_sets(PlanState *planstate, Agg *agg,
 
 	ExplainOpenGroup("Grouping Sets", "Grouping Sets", false, es);
 
-	show_grouping_set_keys(planstate, agg, NULL,
+	show_grouping_set_keys(planstate, agg, (Sort *) agg->sortnode,
 						   context, useprefix, ancestors, es);
 
 	foreach(lc, agg->chain)
 	{
 		Agg		   *aggnode = lfirst(lc);
-		Sort	   *sortnode = (Sort *) aggnode->plan.lefttree;
 
-		show_grouping_set_keys(planstate, aggnode, sortnode,
+		show_grouping_set_keys(planstate, aggnode, (Sort *) aggnode->sortnode,
 							   context, useprefix, ancestors, es);
 	}
 
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 7aebb247d8..b4f53bf77a 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -278,6 +278,7 @@ static void build_hash_table(AggState *aggstate, int setno, long nbuckets);
 static AggStatePerGroup lookup_hash_entry(AggState *aggstate, uint32 hash);
 static void lookup_hash_entries(AggState *aggstate);
 static TupleTableSlot *agg_retrieve_direct(AggState *aggstate);
+static void agg_sort_input(AggState *aggstate);
 static void agg_fill_hash_table(AggState *aggstate);
 static TupleTableSlot *agg_retrieve_hash_table(AggState *aggstate);
 static Datum GetAggInitVal(Datum textInitVal, Oid transtype);
@@ -367,7 +368,7 @@ initialize_phase(AggState *aggstate, int newphase)
 	 */
 	if (newphase > 0 && newphase < aggstate->numphases - 1)
 	{
-		Sort	   *sortnode = aggstate->phases[newphase + 1].sortnode;
+		Sort	   *sortnode = (Sort *)aggstate->phases[newphase + 1].aggnode->sortnode;
 		PlanState  *outerNode = outerPlanState(aggstate);
 		TupleDesc	tupDesc = ExecGetResultType(outerNode);
 
@@ -1594,6 +1595,8 @@ ExecAgg(PlanState *pstate)
 				break;
 			case AGG_PLAIN:
 			case AGG_SORTED:
+				if (!node->input_sorted)
+					agg_sort_input(node);
 				result = agg_retrieve_direct(node);
 				break;
 		}
@@ -1945,6 +1948,45 @@ agg_retrieve_direct(AggState *aggstate)
 	return NULL;
 }
 
+static void
+agg_sort_input(AggState *aggstate)
+{
+	AggStatePerPhase phase = &aggstate->phases[1];
+	TupleDesc	tupDesc;
+	Sort		*sortnode;
+
+	Assert(!aggstate->input_sorted);
+	Assert(phase->aggnode->sortnode);
+
+	sortnode = (Sort *) phase->aggnode->sortnode;
+	tupDesc = ExecGetResultType(outerPlanState(aggstate));
+
+	aggstate->sort_in = tuplesort_begin_heap(tupDesc,
+											 sortnode->numCols,
+											 sortnode->sortColIdx,
+											 sortnode->sortOperators,
+											 sortnode->collations,
+											 sortnode->nullsFirst,
+											 work_mem,
+											 NULL, false);
+	for (;;)
+	{
+		TupleTableSlot *outerslot;
+
+		outerslot = ExecProcNode(outerPlanState(aggstate));
+		if (TupIsNull(outerslot))
+			break;
+
+		tuplesort_puttupleslot(aggstate->sort_in, outerslot);
+	}
+
+	/* Sort the first phase */
+	tuplesort_performsort(aggstate->sort_in);
+
+	/* Mark the input to be sorted */
+	aggstate->input_sorted = true;
+}
+
 /*
  * ExecAgg for hashed case: read input and build hash table
  */
@@ -2127,6 +2169,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	Plan	   *outerPlan;
 	ExprContext *econtext;
 	TupleDesc	scanDesc;
+	Agg			*firstSortAgg;
 	int			numaggs,
 				transno,
 				aggno;
@@ -2171,6 +2214,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	aggstate->grp_firstTuple = NULL;
 	aggstate->sort_in = NULL;
 	aggstate->sort_out = NULL;
+	aggstate->input_sorted = true;
 
 	/*
 	 * phases[0] always exists, but is dummy in sorted/plain mode
@@ -2178,6 +2222,8 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	numPhases = (use_hashing ? 1 : 2);
 	numHashes = (use_hashing ? 1 : 0);
 
+	firstSortAgg = node->aggstrategy == AGG_SORTED ? node : NULL;
+
 	/*
 	 * Calculate the maximum number of grouping sets in any phase; this
 	 * determines the size of some allocations.  Also calculate the number of
@@ -2199,7 +2245,13 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 			 * others add an extra phase.
 			 */
 			if (agg->aggstrategy != AGG_HASHED)
+			{
 				++numPhases;
+
+				if (!firstSortAgg)
+					firstSortAgg = agg;
+
+			}
 			else
 				++numHashes;
 		}
@@ -2208,6 +2260,13 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	aggstate->maxsets = numGroupingSets;
 	aggstate->numphases = numPhases;
 
+	/*
+	 * The first SORTED phase is not sorted, agg need to do its own sort. See
+	 * agg_sort_input(), this can only happen in groupingsets case.
+	 */
+	if (firstSortAgg && firstSortAgg->sortnode)
+		aggstate->input_sorted = false;	
+
 	aggstate->aggcontexts = (ExprContext **)
 		palloc0(sizeof(ExprContext *) * numGroupingSets);
 
@@ -2269,7 +2328,8 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	 * If there are more than two phases (including a potential dummy phase
 	 * 0), input will be resorted using tuplesort. Need a slot for that.
 	 */
-	if (numPhases > 2)
+	if (numPhases > 2 ||
+		!aggstate->input_sorted)
 	{
 		aggstate->sort_slot = ExecInitExtraTupleSlot(estate, scanDesc,
 													 &TTSOpsMinimalTuple);
@@ -2340,20 +2400,11 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	for (phaseidx = 0; phaseidx <= list_length(node->chain); ++phaseidx)
 	{
 		Agg		   *aggnode;
-		Sort	   *sortnode;
 
 		if (phaseidx > 0)
-		{
 			aggnode = list_nth_node(Agg, node->chain, phaseidx - 1);
-			sortnode = castNode(Sort, aggnode->plan.lefttree);
-		}
 		else
-		{
 			aggnode = node;
-			sortnode = NULL;
-		}
-
-		Assert(phase <= 1 || sortnode);
 
 		if (aggnode->aggstrategy == AGG_HASHED
 			|| aggnode->aggstrategy == AGG_MIXED)
@@ -2470,7 +2521,6 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 
 			phasedata->aggnode = aggnode;
 			phasedata->aggstrategy = aggnode->aggstrategy;
-			phasedata->sortnode = sortnode;
 		}
 	}
 
@@ -3559,6 +3609,10 @@ ExecReScanAgg(AggState *node)
 				   sizeof(AggStatePerGroupData) * node->numaggs);
 		}
 
+		/* Reset input_sorted */
+		if (aggnode->sortnode)
+			node->input_sorted = false;
+
 		/* reset to phase 1 */
 		initialize_phase(node, 1);
 
@@ -3566,6 +3620,7 @@ ExecReScanAgg(AggState *node)
 		node->projected_set = -1;
 	}
 
+
 	if (outerPlan->chgParam == NULL)
 		ExecReScan(outerPlan);
 }
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index eaab97f753..04b4c65858 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -992,6 +992,7 @@ _copyAgg(const Agg *from)
 	COPY_BITMAPSET_FIELD(aggParams);
 	COPY_NODE_FIELD(groupingSets);
 	COPY_NODE_FIELD(chain);
+	COPY_NODE_FIELD(sortnode);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e084c3f069..5816d122c1 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -787,6 +787,7 @@ _outAgg(StringInfo str, const Agg *node)
 	WRITE_BITMAPSET_FIELD(aggParams);
 	WRITE_NODE_FIELD(groupingSets);
 	WRITE_NODE_FIELD(chain);
+	WRITE_NODE_FIELD(sortnode);
 }
 
 static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index d5b23a3479..af4fcfe1ee 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -2207,6 +2207,7 @@ _readAgg(void)
 	READ_BITMAPSET_FIELD(aggParams);
 	READ_NODE_FIELD(groupingSets);
 	READ_NODE_FIELD(chain);
+	READ_NODE_FIELD(sortnode);
 
 	READ_DONE();
 }
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index fc25908dc6..d5b34089aa 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1645,6 +1645,7 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags)
 								 NIL,
 								 best_path->path.rows,
 								 0,
+								 NULL,
 								 subplan);
 	}
 	else
@@ -2098,6 +2099,7 @@ create_agg_plan(PlannerInfo *root, AggPath *best_path)
 					NIL,
 					best_path->numGroups,
 					best_path->transitionSpace,
+					NULL,
 					subplan);
 
 	copy_generic_path_info(&plan->plan, (Path *) best_path);
@@ -2159,6 +2161,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
 	List	   *rollups = best_path->rollups;
 	AttrNumber *grouping_map;
 	int			maxref;
+	int			flags = CP_LABEL_TLIST;
 	List	   *chain;
 	ListCell   *lc;
 
@@ -2168,9 +2171,15 @@ 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
+	 * 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.
 	 */
-	subplan = create_plan_recurse(root, best_path->subpath, CP_LABEL_TLIST);
+	if (!best_path->is_sorted)
+		flags = flags | CP_SMALL_TLIST;
+
+	subplan = create_plan_recurse(root, best_path->subpath, flags);
 
 	/*
 	 * Compute the mapping from tleSortGroupRef to column index in the child's
@@ -2230,12 +2239,22 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
 
 			new_grpColIdx = remap_groupColIdx(root, rollup->groupClause);
 
-			if (!rollup->is_hashed && !is_first_sort)
+			if (!rollup->is_hashed)
 			{
-				sort_plan = (Plan *)
-					make_sort_from_groupcols(rollup->groupClause,
-											 new_grpColIdx,
-											 subplan);
+				if (!is_first_sort ||
+					(is_first_sort && !best_path->is_sorted))
+				{
+					sort_plan = (Plan *)
+						make_sort_from_groupcols(rollup->groupClause,
+												 new_grpColIdx,
+												 subplan);
+
+					/*
+					 * Remove stuff we don't need to avoid bloating debug output.
+					 */
+					sort_plan->targetlist = NIL;
+					sort_plan->lefttree = NULL;
+				}
 			}
 
 			if (!rollup->is_hashed)
@@ -2260,16 +2279,8 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
 										 NIL,
 										 rollup->numGroups,
 										 best_path->transitionSpace,
-										 sort_plan);
-
-			/*
-			 * Remove stuff we don't need to avoid bloating debug output.
-			 */
-			if (sort_plan)
-			{
-				sort_plan->targetlist = NIL;
-				sort_plan->lefttree = NULL;
-			}
+										 sort_plan,
+										 NULL);
 
 			chain = lappend(chain, agg_plan);
 		}
@@ -2281,10 +2292,26 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
 	{
 		RollupData *rollup = linitial(rollups);
 		AttrNumber *top_grpColIdx;
+		Plan	   *sort_plan = NULL;
 		int			numGroupCols;
 
 		top_grpColIdx = remap_groupColIdx(root, rollup->groupClause);
 
+		/* the input is not sorted yet */
+		if (!rollup->is_hashed &&
+			!best_path->is_sorted)
+		{
+			sort_plan = (Plan *)
+				make_sort_from_groupcols(rollup->groupClause,
+										 top_grpColIdx,
+										 subplan);
+			/*
+			 * Remove stuff we don't need to avoid bloating debug output.
+			 */
+			sort_plan->targetlist = NIL;
+			sort_plan->lefttree = NULL;
+		}
+
 		numGroupCols = list_length((List *) linitial(rollup->gsets));
 
 		plan = make_agg(build_path_tlist(root, &best_path->path),
@@ -2299,6 +2326,7 @@ create_groupingsets_plan(PlannerInfo *root, GroupingSetsPath *best_path)
 						chain,
 						rollup->numGroups,
 						best_path->transitionSpace,
+						sort_plan,
 						subplan);
 
 		/* Copy cost data from Path to Plan */
@@ -6197,7 +6225,7 @@ make_agg(List *tlist, List *qual,
 		 AggStrategy aggstrategy, AggSplit aggsplit,
 		 int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators, Oid *grpCollations,
 		 List *groupingSets, List *chain, double dNumGroups,
-		 Size transitionSpace, Plan *lefttree)
+		 Size transitionSpace, Plan *sortnode, Plan *lefttree)
 {
 	Agg		   *node = makeNode(Agg);
 	Plan	   *plan = &node->plan;
@@ -6217,6 +6245,7 @@ make_agg(List *tlist, List *qual,
 	node->aggParams = NULL;		/* SS_finalize_plan() will fill this */
 	node->groupingSets = groupingSets;
 	node->chain = chain;
+	node->sortnode = sortnode;
 
 	plan->qual = qual;
 	plan->targetlist = tlist;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index b44efd6314..82a15761b4 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -175,7 +175,8 @@ static void consider_groupingsets_paths(PlannerInfo *root,
 										bool can_hash,
 										grouping_sets_data *gd,
 										const AggClauseCosts *agg_costs,
-										double dNumGroups);
+										double dNumGroups,
+										AggStrategy strat);
 static RelOptInfo *create_window_paths(PlannerInfo *root,
 									   RelOptInfo *input_rel,
 									   PathTarget *input_target,
@@ -4186,6 +4187,14 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
  * it, by combinations of hashing and sorting.  This can be called multiple
  * 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.
  */
 static void
 consider_groupingsets_paths(PlannerInfo *root,
@@ -4195,13 +4204,15 @@ consider_groupingsets_paths(PlannerInfo *root,
 							bool can_hash,
 							grouping_sets_data *gd,
 							const AggClauseCosts *agg_costs,
-							double dNumGroups)
+							double dNumGroups,
+							AggStrategy strat)
 {
 	Query	   *parse = root->parse;
+	Assert(strat == AGG_HASHED || strat == AGG_SORTED);
 
 	/*
-	 * If we're not being offered sorted input, then only consider plans that
-	 * can be done entirely by hashing.
+	 * If strat is AGG_HASHED, then only consider plans that can be done
+	 * entirely by hashing.
 	 *
 	 * We can hash everything if it looks like it'll fit in work_mem. But if
 	 * the input is actually sorted despite not being advertised as such, we
@@ -4210,7 +4221,7 @@ consider_groupingsets_paths(PlannerInfo *root,
 	 * If none of the grouping sets are sortable, then ignore the work_mem
 	 * limit and generate a path anyway, since otherwise we'll just fail.
 	 */
-	if (!is_sorted)
+	if (strat == AGG_HASHED)
 	{
 		List	   *new_rollups = NIL;
 		RollupData *unhashed_rollup = NULL;
@@ -4251,6 +4262,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 */
+			is_sorted = true;
 		}
 
 		hashsize = estimate_hashagg_tablesize(path,
@@ -4348,6 +4361,8 @@ consider_groupingsets_paths(PlannerInfo *root,
 			rollup->hashable = false;
 			rollup->is_hashed = false;
 			new_rollups = lappend(new_rollups, rollup);
+			/* update is_sorted to true */
+			is_sorted = true;
 			strat = AGG_MIXED;
 		}
 
@@ -4359,18 +4374,23 @@ consider_groupingsets_paths(PlannerInfo *root,
 										  strat,
 										  new_rollups,
 										  agg_costs,
-										  dNumGroups));
+										  dNumGroups,
+										  is_sorted));
 		return;
 	}
 
 	/*
-	 * If we have sorted input but nothing we can do with it, bail.
+	 * Strategy is AGG_SORTED but nothing we can do with it, bail.
 	 */
 	if (list_length(gd->rollups) == 0)
 		return;
 
 	/*
-	 * Given sorted input, we try and make two paths: one sorted and one mixed
+	 * Callers consider AGG_SORTED strategy, the first rollup must
+	 * use non-hashed aggregate, 'is_sorted' tells whether the first
+	 * rollup need to do its own sort.
+	 *
+	 * we try and make two paths: one sorted and one mixed
 	 * sort/hash. (We need to try both because hashagg might be disabled, or
 	 * some columns might not be sortable.)
 	 *
@@ -4427,7 +4447,7 @@ consider_groupingsets_paths(PlannerInfo *root,
 
 			/*
 			 * We leave the first rollup out of consideration since it's the
-			 * one that matches the input sort order.  We assign indexes "i"
+			 * one that need to be sorted.  We assign indexes "i"
 			 * to only those entries considered for hashing; the second loop,
 			 * below, must use the same condition.
 			 */
@@ -4516,7 +4536,8 @@ consider_groupingsets_paths(PlannerInfo *root,
 											  AGG_MIXED,
 											  rollups,
 											  agg_costs,
-											  dNumGroups));
+											  dNumGroups,
+											  is_sorted));
 		}
 	}
 
@@ -4532,7 +4553,8 @@ consider_groupingsets_paths(PlannerInfo *root,
 										  AGG_SORTED,
 										  gd->rollups,
 										  agg_costs,
-										  dNumGroups));
+										  dNumGroups,
+										  is_sorted));
 }
 
 /*
@@ -6399,6 +6421,16 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 											  path->pathkeys);
 			if (path == cheapest_path || is_sorted)
 			{
+				if (parse->groupingSets)
+				{
+					/* consider AGG_SORTED strategy */
+					consider_groupingsets_paths(root, grouped_rel,
+												path, is_sorted, can_hash,
+												gd, agg_costs, dNumGroups,
+												AGG_SORTED);
+					continue;
+				}
+
 				/* Sort the cheapest-total path if it isn't already sorted */
 				if (!is_sorted)
 					path = (Path *) create_sort_path(root,
@@ -6407,14 +6439,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 													 root->group_pathkeys,
 													 -1.0);
 
-				/* Now decide what to stick atop it */
-				if (parse->groupingSets)
-				{
-					consider_groupingsets_paths(root, grouped_rel,
-												path, true, can_hash,
-												gd, agg_costs, dNumGroups);
-				}
-				else if (parse->hasAggs)
+				if (parse->hasAggs)
 				{
 					/*
 					 * We have aggregation, possibly with plain GROUP BY. Make
@@ -6514,7 +6539,8 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 			 */
 			consider_groupingsets_paths(root, grouped_rel,
 										cheapest_path, false, true,
-										gd, agg_costs, dNumGroups);
+										gd, agg_costs, dNumGroups,
+										AGG_HASHED);
 		}
 		else
 		{
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index d9ce516211..0feb3363d3 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2983,6 +2983,7 @@ create_agg_path(PlannerInfo *root,
  * 'rollups' is a list of RollupData nodes
  * 'agg_costs' contains cost info about the aggregate functions to be computed
  * 'numGroups' is the estimated total number of groups
+ * 'is_sorted' is the input sorted in the group cols of first rollup
  */
 GroupingSetsPath *
 create_groupingsets_path(PlannerInfo *root,
@@ -2992,7 +2993,8 @@ create_groupingsets_path(PlannerInfo *root,
 						 AggStrategy aggstrategy,
 						 List *rollups,
 						 const AggClauseCosts *agg_costs,
-						 double numGroups)
+						 double numGroups,
+						 bool is_sorted)
 {
 	GroupingSetsPath *pathnode = makeNode(GroupingSetsPath);
 	PathTarget *target = rel->reltarget;
@@ -3010,6 +3012,7 @@ create_groupingsets_path(PlannerInfo *root,
 		subpath->parallel_safe;
 	pathnode->path.parallel_workers = subpath->parallel_workers;
 	pathnode->subpath = subpath;
+	pathnode->is_sorted = is_sorted;
 
 	/*
 	 * Simplify callers by downgrading AGG_SORTED to AGG_PLAIN, and AGG_MIXED
@@ -3061,14 +3064,33 @@ create_groupingsets_path(PlannerInfo *root,
 		 */
 		if (is_first)
 		{
+			Cost	input_startup_cost = subpath->startup_cost;
+			Cost	input_total_cost = subpath->total_cost;
+
+			if (!rollup->is_hashed && !is_sorted && numGroupCols)
+			{
+				Path		sort_path;	/* dummy for result of cost_sort */
+
+				cost_sort(&sort_path, root, NIL,
+						  input_total_cost,
+						  subpath->rows,
+						  subpath->pathtarget->width,
+						  0.0,
+						  work_mem,
+						  -1.0);
+
+				input_startup_cost = sort_path.startup_cost;
+				input_total_cost = sort_path.total_cost;
+			}
+
 			cost_agg(&pathnode->path, root,
 					 aggstrategy,
 					 agg_costs,
 					 numGroupCols,
 					 rollup->numGroups,
 					 having_qual,
-					 subpath->startup_cost,
-					 subpath->total_cost,
+					 input_startup_cost,
+					 input_total_cost,
 					 subpath->rows);
 			is_first = false;
 			if (!rollup->is_hashed)
@@ -3079,7 +3101,7 @@ create_groupingsets_path(PlannerInfo *root,
 			Path		sort_path;	/* dummy for result of cost_sort */
 			Path		agg_path;	/* dummy for result of cost_agg */
 
-			if (rollup->is_hashed || is_first_sort)
+			if (rollup->is_hashed || (is_first_sort && is_sorted))
 			{
 				/*
 				 * Account for cost of aggregation, but don't charge input
diff --git a/src/include/executor/nodeAgg.h b/src/include/executor/nodeAgg.h
index 264916f9a9..66a83b9ac9 100644
--- a/src/include/executor/nodeAgg.h
+++ b/src/include/executor/nodeAgg.h
@@ -277,8 +277,6 @@ typedef struct AggStatePerPhaseData
 	ExprState **eqfunctions;	/* expression returning equality, indexed by
 								 * nr of cols to compare */
 	Agg		   *aggnode;		/* Agg node for phase data */
-	Sort	   *sortnode;		/* Sort node for input ordering for phase */
-
 	ExprState  *evaltrans;		/* evaluation of transition functions  */
 }			AggStatePerPhaseData;
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cd3ddf781f..5e33a368f5 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2083,8 +2083,11 @@ typedef struct AggState
 	AggStatePerGroup *hash_pergroup;	/* grouping set indexed array of
 										 * per-group pointers */
 
+	/* these fields are used in AGG_SORTED and AGG_MIXED */
+	bool		input_sorted;	/* hash table filled yet? */
+
 	/* support for evaluation of agg input expressions: */
-#define FIELDNO_AGGSTATE_ALL_PERGROUPS 34
+#define FIELDNO_AGGSTATE_ALL_PERGROUPS 35
 	AggStatePerGroup *all_pergroups;	/* array of first ->pergroups, than
 										 * ->hash_pergroup */
 	ProjectionInfo *combinedproj;	/* projection machinery */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 0ceb809644..c1e69c808f 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1702,6 +1702,7 @@ typedef struct GroupingSetsPath
 	List	   *rollups;		/* list of RollupData */
 	List	   *qual;			/* quals (HAVING quals), if any */
 	uint64		transitionSpace;	/* for pass-by-ref transition data */
+	bool		is_sorted;		/* input sorted in groupcols of first rollup */
 } GroupingSetsPath;
 
 /*
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 4869fe7b6d..3cd2537e9e 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -818,6 +818,7 @@ typedef struct Agg
 	/* Note: planner provides numGroups & aggParams only in HASHED/MIXED case */
 	List	   *groupingSets;	/* grouping sets to use */
 	List	   *chain;			/* chained Agg/Sort nodes */
+	Plan	   *sortnode;		/* agg does its own sort, only used by grouping sets now */
 } Agg;
 
 /* ----------------
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index e450fe112a..f9f388ba06 100644
--- a/src/include/optimizer/pathnode.h
+++ b/src/include/optimizer/pathnode.h
@@ -217,7 +217,8 @@ extern GroupingSetsPath *create_groupingsets_path(PlannerInfo *root,
 												  AggStrategy aggstrategy,
 												  List *rollups,
 												  const AggClauseCosts *agg_costs,
-												  double numGroups);
+												  double numGroups,
+												  bool is_sorted);
 extern MinMaxAggPath *create_minmaxagg_path(PlannerInfo *root,
 											RelOptInfo *rel,
 											PathTarget *target,
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index 4781201001..5954ff3997 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -55,7 +55,7 @@ extern Agg *make_agg(List *tlist, List *qual,
 					 AggStrategy aggstrategy, AggSplit aggsplit,
 					 int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators, Oid *grpCollations,
 					 List *groupingSets, List *chain, double dNumGroups,
-					 Size transitionSpace, Plan *lefttree);
+					 Size transitionSpace, Plan *sortnode, Plan *lefttree);
 extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node *limitCount);
 
 /*
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index c1f802c88a..12425f46ca 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -366,15 +366,14 @@ explain (costs off)
 select g as alias1, g as alias2
   from generate_series(1,3) g
  group by alias1, rollup(alias2);
-                   QUERY PLAN                   
-------------------------------------------------
+                QUERY PLAN                
+------------------------------------------
  GroupAggregate
-   Group Key: g, g
-   Group Key: g
-   ->  Sort
-         Sort Key: g
-         ->  Function Scan on generate_series g
-(6 rows)
+   Sort Key: g, g
+     Group Key: g, g
+     Group Key: g
+   ->  Function Scan on generate_series g
+(5 rows)
 
 select g as alias1, g as alias2
   from generate_series(1,3) g
@@ -640,15 +639,14 @@ select a, b, sum(v.x)
 -- Test reordering of grouping sets
 explain (costs off)
 select * from gstest1 group by grouping sets((a,b,v),(v)) order by v,b,a;
-                                  QUERY PLAN                                  
-------------------------------------------------------------------------------
+                                QUERY PLAN                                 
+---------------------------------------------------------------------------
  GroupAggregate
-   Group Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1
-   Group Key: "*VALUES*".column3
-   ->  Sort
-         Sort Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1
-         ->  Values Scan on "*VALUES*"
-(6 rows)
+   Sort Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1
+     Group Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1
+     Group Key: "*VALUES*".column3
+   ->  Values Scan on "*VALUES*"
+(5 rows)
 
 -- Agg level check. This query should error out.
 select (select grouping(a,b) from gstest2) from gstest2 group by a,b;
@@ -723,13 +721,12 @@ explain (costs off)
             QUERY PLAN            
 ----------------------------------
  GroupAggregate
-   Group Key: a
-   Group Key: ()
+   Sort Key: a
+     Group Key: a
+     Group Key: ()
    Filter: (a IS DISTINCT FROM 1)
-   ->  Sort
-         Sort Key: a
-         ->  Seq Scan on gstest2
-(7 rows)
+   ->  Seq Scan on gstest2
+(6 rows)
 
 select v.c, (select count(*) from gstest2 group by () having v.c)
   from (values (false),(true)) v(c) order by v.c;
@@ -1018,18 +1015,17 @@ explain (costs off) select a, b, grouping(a,b), sum(v), count(*), max(v)
 explain (costs off)
   select a, b, grouping(a,b), array_agg(v order by v)
     from gstest1 group by cube(a,b);
-                        QUERY PLAN                        
-----------------------------------------------------------
+                      QUERY PLAN                       
+-------------------------------------------------------
  GroupAggregate
-   Group Key: "*VALUES*".column1, "*VALUES*".column2
-   Group Key: "*VALUES*".column1
-   Group Key: ()
+   Sort Key: "*VALUES*".column1, "*VALUES*".column2
+     Group Key: "*VALUES*".column1, "*VALUES*".column2
+     Group Key: "*VALUES*".column1
+     Group Key: ()
    Sort Key: "*VALUES*".column2
      Group Key: "*VALUES*".column2
-   ->  Sort
-         Sort Key: "*VALUES*".column1, "*VALUES*".column2
-         ->  Values Scan on "*VALUES*"
-(9 rows)
+   ->  Values Scan on "*VALUES*"
+(8 rows)
 
 -- unsortable cases
 select unsortable_col, count(*)
@@ -1071,11 +1067,10 @@ explain (costs off)
    Sort Key: (GROUPING(unhashable_col, unsortable_col)), (sum(v))
    ->  MixedAggregate
          Hash Key: unsortable_col
-         Group Key: unhashable_col
-         ->  Sort
-               Sort Key: unhashable_col
-               ->  Seq Scan on gstest4
-(8 rows)
+         Sort Key: unhashable_col
+           Group Key: unhashable_col
+         ->  Seq Scan on gstest4
+(7 rows)
 
 select unhashable_col, unsortable_col,
        grouping(unhashable_col, unsortable_col),
@@ -1114,11 +1109,10 @@ explain (costs off)
    Sort Key: (GROUPING(unhashable_col, unsortable_col)), (sum(v))
    ->  MixedAggregate
          Hash Key: v, unsortable_col
-         Group Key: v, unhashable_col
-         ->  Sort
-               Sort Key: v, unhashable_col
-               ->  Seq Scan on gstest4
-(8 rows)
+         Sort Key: v, unhashable_col
+           Group Key: v, unhashable_col
+         ->  Seq Scan on gstest4
+(7 rows)
 
 -- empty input: first is 0 rows, second 1, third 3 etc.
 select a, b, sum(v), count(*) from gstest_empty group by grouping sets ((a,b),a);
@@ -1366,19 +1360,18 @@ explain (costs off)
 BEGIN;
 SET LOCAL enable_hashagg = false;
 EXPLAIN (COSTS OFF) SELECT a, b, count(*), max(a), max(b) FROM gstest3 GROUP BY GROUPING SETS(a, b,()) ORDER BY a, b;
-              QUERY PLAN               
----------------------------------------
+           QUERY PLAN            
+---------------------------------
  Sort
    Sort Key: a, b
    ->  GroupAggregate
-         Group Key: a
-         Group Key: ()
+         Sort Key: a
+           Group Key: a
+           Group Key: ()
          Sort Key: b
            Group Key: b
-         ->  Sort
-               Sort Key: a
-               ->  Seq Scan on gstest3
-(10 rows)
+         ->  Seq Scan on gstest3
+(9 rows)
 
 SELECT a, b, count(*), max(a), max(b) FROM gstest3 GROUP BY GROUPING SETS(a, b,()) ORDER BY a, b;
  a | b | count | max | max 
@@ -1549,22 +1542,21 @@ explain (costs off)
          count(hundred), count(thousand), count(twothousand),
          count(*)
     from tenk1 group by grouping sets (unique1,twothousand,thousand,hundred,ten,four,two);
-          QUERY PLAN           
--------------------------------
+         QUERY PLAN         
+----------------------------
  MixedAggregate
    Hash Key: two
    Hash Key: four
    Hash Key: ten
    Hash Key: hundred
-   Group Key: unique1
+   Sort Key: unique1
+     Group Key: unique1
    Sort Key: twothousand
      Group Key: twothousand
    Sort Key: thousand
      Group Key: thousand
-   ->  Sort
-         Sort Key: unique1
-         ->  Seq Scan on tenk1
-(13 rows)
+   ->  Seq Scan on tenk1
+(12 rows)
 
 explain (costs off)
   select unique1,
@@ -1572,18 +1564,17 @@ explain (costs off)
          count(hundred), count(thousand), count(twothousand),
          count(*)
     from tenk1 group by grouping sets (unique1,hundred,ten,four,two);
-          QUERY PLAN           
--------------------------------
+       QUERY PLAN        
+-------------------------
  MixedAggregate
    Hash Key: two
    Hash Key: four
    Hash Key: ten
    Hash Key: hundred
-   Group Key: unique1
-   ->  Sort
-         Sort Key: unique1
-         ->  Seq Scan on tenk1
-(9 rows)
+   Sort Key: unique1
+     Group Key: unique1
+   ->  Seq Scan on tenk1
+(8 rows)
 
 set work_mem = '384kB';
 explain (costs off)
@@ -1592,21 +1583,20 @@ explain (costs off)
          count(hundred), count(thousand), count(twothousand),
          count(*)
     from tenk1 group by grouping sets (unique1,twothousand,thousand,hundred,ten,four,two);
-          QUERY PLAN           
--------------------------------
+         QUERY PLAN         
+----------------------------
  MixedAggregate
    Hash Key: two
    Hash Key: four
    Hash Key: ten
    Hash Key: hundred
    Hash Key: thousand
-   Group Key: unique1
+   Sort Key: unique1
+     Group Key: unique1
    Sort Key: twothousand
      Group Key: twothousand
-   ->  Sort
-         Sort Key: unique1
-         ->  Seq Scan on tenk1
-(12 rows)
+   ->  Seq Scan on tenk1
+(11 rows)
 
 -- check collation-sensitive matching between grouping expressions
 -- (similar to a check for aggregates, but there are additional code
-- 
2.21.1

--4ms6w442s2ji2wqe
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="0002-fixes.patch"