From b4c2a801aa802e71b8a822f5e1cd163463e97e26 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sat, 31 Mar 2018 19:49:38 +0200
Subject: [PATCH] comments

---
 src/backend/commands/explain.c             |   4 +
 src/backend/executor/nodeIncrementalSort.c | 118 ++++++++++++++++++++---------
 src/backend/optimizer/path/costsize.c      |  54 ++++++++-----
 src/backend/optimizer/path/pathkeys.c      |   7 +-
 src/backend/optimizer/plan/createplan.c    |  14 ++++
 src/backend/optimizer/plan/planagg.c       |   1 -
 src/backend/optimizer/plan/planner.c       |   6 +-
 src/backend/optimizer/util/pathnode.c      |   5 ++
 8 files changed, 155 insertions(+), 54 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index edd71ae..8533f36 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2016,6 +2016,10 @@ show_sort_keys(SortState *sortstate, List *ancestors, ExplainState *es)
 	Sort	   *plan = (Sort *) sortstate->ss.ps.plan;
 	int			presortedCols;
 
+	/*
+	 * XXX This seems unnecessary. In which case do we call show_sort_keys
+	 * for incremental sort? Even the comment says it's for Sort only.
+	 */
 	if (IsA(plan, IncrementalSort))
 		presortedCols = ((IncrementalSort *) plan)->presortedCols;
 	else
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 1f5e41f..b95379b 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -5,45 +5,49 @@
  *
  * DESCRIPTION
  *
- *		Incremental sort is a specially optimized kind of multikey sort used
- *		when the input is already presorted by a prefix of the required keys
- *		list.  Thus, when it's required to sort by (key1, key2 ... keyN) and
- *		result is already sorted by (key1, key2 ... keyM), M < N, we sort groups
- *		where values of (key1, key2 ... keyM) are equal.
+ *		Incremental sort is an optimized variant of multikey sort for cases
+ *      when the input is already sorted by a prefix of the sort keys.  For
+ *		example when a sort by (key1, key2 ... keyN) is requested, and the
+ *		input is already sorted by (key1, key2 ... keyM), M < N, we can
+ *      divide the input into groups where keys (key1, ... keyM) are equal,
+ *      and only sort on the remaining columns.
  *
- *		Consider the following example.  We have input tuples consisting from
- *		two integers (x, y) already presorted by x, while it's required to
- *		sort them by x and y.  Let input tuples be following.
+ *		Consider the following example.  We have input tuples consisting of
+ *		two integers (X, Y) already presorted by X, while it's required to
+ *		sort them by both X and Y.  Let input tuples be following.
  *
  *		(1, 5)
  *		(1, 2)
- *		(2, 10)
+ *		(2, 9)
  *		(2, 1)
  *		(2, 5)
  *		(3, 3)
  *		(3, 7)
  *
- *		Incremental sort algorithm would sort by y following groups, which have
- *		equal x, individually:
+ *		Incremental sort algorithm would split the input into the following
+ *      groups, which have equal X, and then sort them by Y individually:
+ *
  *			(1, 5) (1, 2)
- *			(2, 10) (2, 1) (2, 5)
+ *			(2, 9) (2, 1) (2, 5)
  *			(3, 3) (3, 7)
  *
  *		After sorting these groups and putting them altogether, we would get
- *		following tuple set which is actually sorted by x and y.
+ *		the following result which is sorted by X and Y, as requested:
  *
  *		(1, 2)
  *		(1, 5)
  *		(2, 1)
  *		(2, 5)
- *		(2, 10)
+ *		(2, 9)
  *		(3, 3)
  *		(3, 7)
  *
- *		Incremental sort is faster than full sort on large datasets.  But
- *		the case of most huge benefit of incremental sort is queries with
- *		LIMIT because incremental sort can return first tuples without reading
- *		whole input dataset.
+ *		Incremental sort may be more efficient than plain sort, parcitularly
+ *      on large datasets, as it reduces the amount of data to sort at once,
+ *      making it more likely it fits into work_mem (eliminating the need to
+ *      spill to disk).  But the main advantage of incremental sort is that
+ *      it can start producing rows early, before sorting the whole dataset,
+ *      which is a significant benefit especially for queries with LIMIT.
  *
  * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
@@ -112,18 +116,27 @@ preparePresortedCols(IncrementalSortState *node)
 
 /*
  * Check if first "presortedCols" sort values are equal.
+ *
+ * XXX I find the name somewhat confusing, because "cmp" is usually used
+ * for comparators, i.e. functions that return -1/0/1. I suggest something
+ * like "samePrefixGroup" or something like that.
  */
 static bool
 cmpSortPresortedCols(IncrementalSortState *node, TupleTableSlot *a,
 															TupleTableSlot *b)
 {
-	int n, i;
+	int presortedCols, i;
 
 	Assert(IsA(node->ss.ps.plan, IncrementalSort));
 
-	n = ((IncrementalSort *) node->ss.ps.plan)->presortedCols;
+	presortedCols = ((IncrementalSort *) node->ss.ps.plan)->presortedCols;
 
-	for (i = n - 1; i >= 0; i--)
+	/*
+	 * We do assume the input is sorted by keys (0, ... n), which means
+	 * the tail keys are more likely to change. So we do the comparison
+	 * from the end, to minimize the number of function calls.
+	 */
+	for (i = presortedCols - 1; i >= 0; i--)
 	{
 		Datum				datumA,
 							datumB,
@@ -171,6 +184,12 @@ cmpSortPresortedCols(IncrementalSortState *node, TupleTableSlot *a,
  * to cope this problem we don't copy pivot tuple before the group contains
  * at least MIN_GROUP_SIZE of tuples.  Surely, it might reduce efficiency of
  * incremental sort, but it reduces the probability of regression.
+ *
+ * XXX I suppose this is not just about copying the tuples into the slot, but
+ * about frequently sorting tiny amounts of data (tuplesort overhead)?
+ *
+ * XXX Fixed-size limit seems like a bad idea, for example when the subplan
+ * is expected to produce only very few tuples (less than 32) at high cost.
  */
 #define MIN_GROUP_SIZE 32
 
@@ -205,6 +224,9 @@ ExecIncrementalSort(PlanState *pstate)
 	TupleDesc			tupDesc;
 	int64				nTuples = 0;
 
+	/* XXX ExecSort does this, I don't see why ExecIncrementalSort shouldn't */
+	CHECK_FOR_INTERRUPTS();
+
 	/*
 	 * get state info from node
 	 */
@@ -216,7 +238,9 @@ ExecIncrementalSort(PlanState *pstate)
 	tuplesortstate = (Tuplesortstate *) node->tuplesortstate;
 
 	/*
-	 * Return next tuple from sorted set if any.
+	 * Return next tuple from the current sorted group set if available.
+	 * If there are no more tuples in the current group, we need to try
+	 * to fetch more tuples from the input and build another group.
 	 */
 	if (node->sort_Done)
 	{
@@ -228,8 +252,10 @@ ExecIncrementalSort(PlanState *pstate)
 	}
 
 	/*
-	 * If first time through, read all tuples from outer plan and pass them to
-	 * tuplesort.c. Subsequent calls just fetch tuples from tuplesort.
+	 * First time through or no tuples in the current group. Read next
+	 * batch of tuples from the outer plan and pass them to tuplesort.c.
+	 * Subsequent calls just fetch tuples from tuplesort, until the group
+	 * is exhausted, at which point we build the next group.
 	 */
 
 	SO1_printf("ExecIncrementalSort: %s\n",
@@ -241,15 +267,12 @@ ExecIncrementalSort(PlanState *pstate)
 	 */
 	estate->es_direction = ForwardScanDirection;
 
-	/*
-	 * Initialize tuplesort module.
-	 */
-	SO1_printf("ExecIncrementalSort: %s\n",
-			   "calling tuplesort_begin");
-
 	outerNode = outerPlanState(node);
 	tupDesc = ExecGetResultType(outerNode);
 
+	/*
+	 * Initialize tuplesort module (needed only before the first group).
+	 */
 	if (node->tuplesortstate == NULL)
 	{
 		/*
@@ -259,12 +282,17 @@ ExecIncrementalSort(PlanState *pstate)
 		 */
 		preparePresortedCols(node);
 
+		SO1_printf("ExecIncrementalSort: %s\n",
+				   "calling tuplesort_begin_heap");
+
 		/*
 		 * Pass all the columns to tuplesort.  We pass to tuple sort groups
 		 * of at least MIN_GROUP_SIZE size.  Thus, these groups doesn't
 		 * necessary have equal value of the first column.  We unlikely will
 		 * have huge groups with incremental sort.  Therefore usage of
 		 * abbreviated keys would be likely a waste of time.
+		 *
+		 * XXX The claim about abbreviated keys seems rather dubious, IMHO.
 		 */
 		tuplesortstate = tuplesort_begin_heap(
 									tupDesc,
@@ -290,7 +318,7 @@ ExecIncrementalSort(PlanState *pstate)
 	if (node->bounded)
 		tuplesort_set_bound(tuplesortstate, node->bound - node->bound_Done);
 
-	/* Put saved tuple to tuplesort if any */
+	/* If we got a left-over tuple from the last group, pass it to tuplesort. */
 	if (!TupIsNull(node->grpPivotSlot))
 	{
 		tuplesort_puttupleslot(tuplesortstate, node->grpPivotSlot);
@@ -312,19 +340,41 @@ ExecIncrementalSort(PlanState *pstate)
 			break;
 		}
 
-		/* Put next group of presorted data to the tuplesort */
+		/*
+		 * Accumulate the next group of presorted tuples for tuplesort.
+		 * We always accumulate at least MIN_GROUP_SIZE tuples, and only
+		 * then we start to compare the prefix keys.
+		 *
+		 * The last tuple is kept as a pivot, so that we can determine if
+		 * the subsequent tuples have the same prefix key (same group).
+		 */
 		if (nTuples < MIN_GROUP_SIZE)
 		{
 			tuplesort_puttupleslot(tuplesortstate, slot);
 
-			/* Save last tuple in minimal group */
+			/* Keep the last tuple in minimal group as a pivot. */
 			if (nTuples == MIN_GROUP_SIZE - 1)
 				ExecCopySlot(node->grpPivotSlot, slot);
 			nTuples++;
 		}
 		else
 		{
-			/* Iterate while presorted cols are the same as in saved tuple */
+			/*
+			 * Iterate while presorted cols are the same as in saved tuple
+			 *
+			 * After accumulating at least MIN_GROUP_SIZE tuples (we don't
+			 * know how many groups are there in that set), we need to keep
+			 * accumulating until we reach the end of the group. Only then
+			 * we can do the sort and output all the tuples.
+			 *
+			 * We compare the prefix keys to the pivot - if the prefix keys
+			 * are the same the tuple belongs to the same group, so we pass
+			 * it to the tuplesort.
+			 *
+			 * If the prefix differs, we've reached the end of the group. We
+			 * need to keep the last tuple, so we copy it into the pivot slot
+			 * (it does not serve as pivot, though).
+			 */
 			if (cmpSortPresortedCols(node, node->grpPivotSlot, slot))
 			{
 				tuplesort_puttupleslot(tuplesortstate, slot);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index e8cfdd8..52956a8 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1616,13 +1616,6 @@ cost_recursive_union(Path *runion, Path *nrterm, Path *rterm)
  *	  Determines and returns the cost of sorting a relation, including
  *	  the cost of reading the input data.
  *
- * Sort could be either full sort of relation or incremental sort when we already
- * have data presorted by some of required pathkeys.  In the second case
- * we estimate number of groups which source data is divided to by presorted
- * pathkeys.  And then estimate cost of sorting each individual group assuming
- * data is divided into group uniformly.  Also, if LIMIT is specified then
- * we have to pull from source and sort only some of total groups.
- *
  * If the total volume of data to sort is less than sort_mem, we will do
  * an in-memory sort, which requires no I/O and about t*log2(t) tuple
  * comparisons for t tuples.
@@ -1648,6 +1641,16 @@ cost_recursive_union(Path *runion, Path *nrterm, Path *rterm)
  * specifying nonzero comparison_cost; typically that's used for any extra
  * work that has to be done to prepare the inputs to the comparison operators.
  *
+ * The sort may also be incremental, when the input data is already sorted
+ * by a prefix of the requested pathkeys.  In that case we estimate the
+ * number of groups the input data is divided into (by the prefix keys), and
+ * then apply the same costing criteria as for regular sort.  For example the
+ * sort_mem limit is applied on per-group size (assuming average group size),
+ * not the total volume of data.
+ *
+ * If LIMIT is specified, incremental sort only needs to pull and sort
+ * a subset of the input data, unlike the regular sort.
+ *
  * 'pathkeys' is a list of sort keys
  * 'presorted_keys' is a number of pathkeys already presorted in given path
  * 'input_startup_cost' is the startup cost for reading the input data
@@ -1687,6 +1690,7 @@ cost_sort(Path *path, PlannerInfo *root,
 
 	if (!enable_sort)
 		startup_cost += disable_cost;
+
 	if (!enable_incrementalsort)
 		presorted_keys = 0;
 
@@ -1749,6 +1753,19 @@ cost_sort(Path *path, PlannerInfo *root,
 		 */
 		group_input_bytes = 1.5 * input_bytes / num_groups;
 		group_tuples = 1.5 * tuples / num_groups;
+
+		/*
+		 * We want to be sure the cost of a sort is never estimated as zero, even
+		 * if passed-in tuple count is zero.  Besides, mustn't do log(0)...
+		 *
+		 * XXX Same protection as for tuples, at the beginning. And we don't need
+		 * to worry about LOG2() below.
+		 *
+		 * XXX Probably should re-evaluate group_input_bytes, but the difference
+		 * is going to be tiny.
+		 */
+		if (group_tuples < 2.0)
+			group_tuples = 2.0;
 	}
 	else
 	{
@@ -1757,6 +1774,10 @@ cost_sort(Path *path, PlannerInfo *root,
 		group_tuples = tuples;
 	}
 
+	/*
+	 * XXX Can it actually happen that the first condition is true,
+	 * while the second one is false? I don't think so.
+	 */
 	if (output_bytes > sort_mem_bytes && group_input_bytes > sort_mem_bytes)
 	{
 		/*
@@ -1799,15 +1820,8 @@ cost_sort(Path *path, PlannerInfo *root,
 	}
 	else
 	{
-		/*
-		 * We'll use plain quicksort on all the input tuples.  If it appears
-		 * that we expect less than two tuples per sort group then assume
-		 * logarithmic part of estimate to be 1.
-		 */
-		if (group_tuples >= 2.0)
-			group_cost = comparison_cost * group_tuples * LOG2(group_tuples);
-		else
-			group_cost = comparison_cost * group_tuples;
+		/* We'll use plain quicksort on all the input tuples. */
+		group_cost = comparison_cost * group_tuples * LOG2(group_tuples);
 	}
 
 	/* Add per group cost of fetching tuples from input */
@@ -1832,7 +1846,13 @@ cost_sort(Path *path, PlannerInfo *root,
 	 */
 	run_cost += cpu_operator_cost * tuples;
 
-	/* Extra costs of incremental sort */
+	/*
+	 * Extra costs of incremental sort
+	 *
+	 * XXX This pretty much implies there are cases where incremental sort
+	 * is costed as more expensive than plain sort. The difference may be
+	 * fairly small, but if the groups are tiny it may be noticeable.
+	 */
 	if (presorted_keys > 0)
 	{
 		/*
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 57fe52d..506d898 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -27,7 +27,6 @@
 #include "optimizer/paths.h"
 #include "optimizer/tlist.h"
 #include "utils/lsyscache.h"
-#include "utils/selfuncs.h"
 
 
 static bool pathkey_is_redundant(PathKey *new_pathkey, List *pathkeys);
@@ -1648,6 +1647,12 @@ pathkeys_useful_for_ordering(List *query_pathkeys, List *pathkeys)
 	}
 	else
 	{
+		/*
+		 * XXX This seems really strange. Why should we even consider the GUC
+		 * here? It's supposed to be a generic utility function. That belongs
+		 * to costsize.c only, I think. No other function here does something
+		 * like that, which is illustrated by having to include cost.h.
+		 */
 		if (enable_incrementalsort)
 		{
 			/*
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 9ca06c7..74230ec 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1180,6 +1180,13 @@ create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path)
 					  numsortkeys * sizeof(bool)) == 0);
 
 		/* Now, insert a Sort node if subplan isn't sufficiently ordered */
+
+		/*
+		 * XXX This seems wrong, as it builds an incremental sort whenever
+		 * there's at least one prefix column, even when a regular sort
+		 * would be cheaper.
+		 */
+
 		if (!pathkeys_common_contained_in(pathkeys, subpath->pathkeys,
 										  &n_common_pathkeys))
 		{
@@ -1564,6 +1571,13 @@ create_gather_merge_plan(PlannerInfo *root, GatherMergePath *best_path)
 
 
 	/* Now, insert a Sort node if subplan isn't sufficiently ordered */
+
+	/*
+	 * XXX This seems wrong, as it builds an incremental sort whenever
+	 * there's at least one prefix column, even when a regular sort
+	 * would be cheaper.
+	 */
+
 	if (!pathkeys_common_contained_in(pathkeys, best_path->subpath->pathkeys,
 									  &n_common_pathkeys))
 	{
diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index 308f60b..95cbffb 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -44,7 +44,6 @@
 #include "parser/parse_clause.h"
 #include "rewrite/rewriteManip.h"
 #include "utils/lsyscache.h"
-#include "utils/selfuncs.h"
 #include "utils/syscache.h"
 
 
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 39dabce..fa312e9 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6209,7 +6209,11 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 									root->group_pathkeys, path->pathkeys);
 			if (path == cheapest_path || n_useful_pathkeys > 0)
 			{
-				/* Sort the cheapest-total path if it isn't already sorted */
+				/* Sort the cheapest-total path if it isn't already sorted
+				 *
+				 * XXX This comment is obviously stale, as it's not about
+				 * cheapest-total path, but about paths sorted by prefix.
+				 */
 				if (n_useful_pathkeys < list_length(root->group_pathkeys))
 					path = (Path *) create_sort_path(root,
 													 grouped_rel,
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 2202e97..62f39c0 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1368,6 +1368,11 @@ create_merge_append_path(PlannerInfo *root,
 		pathnode->path.parallel_safe = pathnode->path.parallel_safe &&
 			subpath->parallel_safe;
 
+
+		/*
+		 * XXX Same issue as in createplan.c - we always create incremental
+		 * sort, even if plain sort would be cheaper.
+		 */
 		if (pathkeys_common_contained_in(pathkeys, subpath->pathkeys,
 										 &n_common_pathkeys))
 		{
-- 
2.9.5

