From 57f30d60158398bd57f02ef00800d3f63c6ab12f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tv@fuzzy.cz>
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

