v50-0006-cleanup.patch

From 753184e345638b0c681ba6ba1d4b3a2bf7b4c570 Mon Sep 17 00:00:00 2001
From: James Coleman <jtc331@gmail.com>
Date: Wed, 1 Apr 2020 08:53:18 -0400
Subject: [PATCH v50 6/6] cleanup

---
 src/backend/optimizer/path/allpaths.c | 13 ++++-
 src/backend/optimizer/plan/planner.c  | 81 +++++++++++++++++----------
 2 files changed, 60 insertions(+), 34 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 006924d4a6..4a72dcf8bc 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2842,7 +2842,10 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r
 			Path	   *subpath = (Path *) lfirst(lc2);
 			GatherMergePath *path;
 
-			/* path has no ordering at all, can't use incremental sort */
+			/*
+			 * If the path has no ordering at all, then we can't use either
+			 * incremental sort or rely on implict sorting with a gather merge.
+			 */
 			if (subpath->pathkeys == NIL)
 				continue;
 
@@ -2867,8 +2870,6 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r
 				continue;
 			}
 
-			Assert(!is_sorted);
-
 			/*
 			 * Consider regular sort for the cheapest partial path (for each
 			 * useful pathkeys). We know the path is not sorted, because we'd
@@ -2911,6 +2912,12 @@ generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_r
 			{
 				Path	   *tmp;
 
+				/*
+				 * We should have already excluded pathkeys of length 1 because
+				 * then presorted_keys > 0 would imply is_sorted was true.
+				 */
+				Assert(list_length(useful_pathkeys) != 1);
+
 				tmp = (Path *) create_incremental_sort_path(root,
 															rel,
 															subpath,
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 1cfbd88eec..9608fdaec8 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5096,8 +5096,12 @@ create_ordered_paths(PlannerInfo *root,
 		 *
 		 * XXX This is probably duplicate with the paths we already generate
 		 * in generate_useful_gather_paths in apply_scanjoin_target_to_paths.
+		 *
+		 * We can also skip the entire loop when we only have a single-item
+		 * sort_pathkeys because then we can't possibly have a presorted
+		 * prefix of the list without having the list be fully sorted.
 		 */
-		if (enable_incrementalsort)
+		if (enable_incrementalsort && list_length(root->sort_pathkeys) > 1)
 		{
 			ListCell   *lc;
 
@@ -6509,8 +6513,10 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 			bool		is_sorted;
 			int			presorted_keys;
 
-			is_sorted = pathkeys_contained_in(root->group_pathkeys,
-											  path->pathkeys);
+			is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
+													 path->pathkeys,
+													 &presorted_keys);
+
 			if (path == cheapest_path || is_sorted)
 			{
 				/* Sort the cheapest-total path if it isn't already sorted */
@@ -6578,17 +6584,16 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 			/* Restore the input path (we might have added Sort on top). */
 			path = path_original;
 
-			is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
-													 path->pathkeys,
-													 &presorted_keys);
-
-			/* We've already skipped fully sorted paths above. */
-			Assert(!is_sorted);
-
 			/* no shared prefix, no point in building incremental sort */
 			if (presorted_keys == 0)
 				continue;
 
+			/*
+			 * We should have already excluded pathkeys of length 1 because
+			 * then presorted_keys > 0 would imply is_sorted was true.
+			 */
+			Assert(list_length(root->group_pathkeys) != 1);
+
 			path = (Path *) create_incremental_sort_path(root,
 														 grouped_rel,
 														 path,
@@ -6655,8 +6660,9 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 				bool		is_sorted;
 				int			presorted_keys;
 
-				is_sorted = pathkeys_contained_in(root->group_pathkeys,
-												  path->pathkeys);
+				is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
+														 path->pathkeys,
+														 &presorted_keys);
 
 				/*
 				 * Insert a Sort node, if required.  But there's no point in
@@ -6705,17 +6711,16 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel,
 				/* Restore the input path (we might have added Sort on top). */
 				path = path_original;
 
-				is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
-														 path->pathkeys,
-														 &presorted_keys);
-
-				/* We've already skipped fully sorted paths above. */
-				Assert(!is_sorted);
-
 				/* no shared prefix, not point in building incremental sort */
 				if (presorted_keys == 0)
 					continue;
 
+				/*
+				 * We should have already excluded pathkeys of length 1 because
+				 * then presorted_keys > 0 would imply is_sorted was true.
+				 */
+				Assert(list_length(root->group_pathkeys) != 1);
+
 				path = (Path *) create_incremental_sort_path(root,
 															 grouped_rel,
 															 path,
@@ -7015,8 +7020,14 @@ create_partial_grouping_paths(PlannerInfo *root,
 			}
 		}
 
-		/* Consider incremental sort on all partial paths, if enabled. */
-		if (enable_incrementalsort)
+		/*
+		 * Consider incremental sort on all partial paths, if enabled.
+		 *
+		 * We can also skip the entire loop when we only have a single-item
+		 * group_pathkeys because then we can't possibly have a presorted
+		 * prefix of the list without having the list be fully sorted.
+		 */
+		if (enable_incrementalsort && list_length(root->group_pathkeys) > 1)
 		{
 			foreach(lc, input_rel->pathlist)
 			{
@@ -7078,8 +7089,10 @@ create_partial_grouping_paths(PlannerInfo *root,
 			bool		is_sorted;
 			int			presorted_keys;
 
-			is_sorted = pathkeys_contained_in(root->group_pathkeys,
-											  path->pathkeys);
+			is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
+													 path->pathkeys,
+													 &presorted_keys);
+
 			if (path == cheapest_partial_path || is_sorted)
 			{
 				/* Sort the cheapest partial path, if it isn't already */
@@ -7123,17 +7136,16 @@ create_partial_grouping_paths(PlannerInfo *root,
 			/* Restore the input path (we might have added Sort on top). */
 			path = path_original;
 
-			is_sorted = pathkeys_count_contained_in(root->group_pathkeys,
-													 path->pathkeys,
-													 &presorted_keys);
-
-			/* We've already skipped fully sorted paths above. */
-			Assert(!is_sorted);
-
 			/* no shared prefix, not point in building incremental sort */
 			if (presorted_keys == 0)
 				continue;
 
+			/*
+			 * We should have already excluded pathkeys of length 1 because
+			 * then presorted_keys > 0 would imply is_sorted was true.
+			 */
+			Assert(list_length(root->group_pathkeys) != 1);
+
 			path = (Path *) create_incremental_sort_path(root,
 														 partially_grouped_rel,
 														 path,
@@ -7289,7 +7301,14 @@ gather_grouping_paths(PlannerInfo *root, RelOptInfo *rel)
 		add_path(rel, path);
 	}
 
-	if (!enable_incrementalsort)
+	/*
+	 * Consider incremental sort on all partial paths, if enabled.
+	 *
+	 * We can also skip the entire loop when we only have a single-item
+	 * group_pathkeys because then we can't possibly have a presorted
+	 * prefix of the list without having the list be fully sorted.
+	 */
+	if (!enable_incrementalsort || list_length(root->group_pathkeys) == 1)
 		return;
 
 	/* also consider incremental sort on partial paths, if enabled */
-- 
2.17.1