From 4a84528d96b61b5a4828b8735f5418a79bcf526e 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/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

