From df9c30e1e8a7761a47ea62ef9e768a9ce7ac4b87 Mon Sep 17 00:00:00 2001
From: James Coleman <jtc331@gmail.com>
Date: Sat, 28 Mar 2020 22:35:49 -0400
Subject: [PATCH 3/4] explain fixes

---
 src/backend/commands/explain.c                | 71 +++++++++---------
 src/backend/executor/nodeIncrementalSort.c    | 72 +++++++++----------
 src/include/nodes/execnodes.h                 |  2 +-
 src/include/utils/tuplesort.h                 | 11 +--
 .../regress/expected/incremental_sort.out     | 18 ++---
 5 files changed, 89 insertions(+), 85 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 85d7bcb78f..8a0dcf09b0 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2713,26 +2713,41 @@ show_sort_info(SortState *sortstate, ExplainState *es)
  */
 static void
 show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
-								 const char *groupLabel, ExplainState *es)
+								 const char *groupLabel, bool indent, ExplainState *es)
 {
 	ListCell   *methodCell;
-	int			methodCount = list_length(groupInfo->sortMethods);
+	List	   *methodNames = NIL;
+
+	/* Generate a list of sort methods used across all groups. */
+	for (int bit = 0; bit < sizeof(Size); ++bit)
+	{
+		if (groupInfo->sortMethods & (1 << bit))
+		{
+			TuplesortMethod sortMethod = (1 << bit);
+			const char *methodName;
+
+			methodName = tuplesort_method_name(sortMethod);
+			methodNames = lappend(methodNames, unconstify(char *, methodName));
+		}
+	}
 
 	if (es->format == EXPLAIN_FORMAT_TEXT)
 	{
-		appendStringInfoSpaces(es->str, es->indent * 2);
-		appendStringInfo(es->str, "%s Groups: %ld (Methods: ", groupLabel,
+		if (indent)
+			appendStringInfoSpaces(es->str, es->indent * 2);
+		appendStringInfo(es->str, "%s Groups: %ld Sort Method", groupLabel,
 						 groupInfo->groupCount);
-		foreach(methodCell, groupInfo->sortMethods)
+		/* plural/singular based on methodNames size */
+		if (list_length(methodNames) > 1)
+			appendStringInfo(es->str, "s: ");
+		else
+			appendStringInfo(es->str, ": ");
+		foreach(methodCell, methodNames)
 		{
-			const char *sortMethodName;
-
-			sortMethodName = tuplesort_method_name(methodCell->int_value);
-			appendStringInfo(es->str, "%s", sortMethodName);
-			if (foreach_current_index(methodCell) < methodCount - 1)
+			appendStringInfo(es->str, "%s", (char *) methodCell->ptr_value);
+			if (foreach_current_index(methodCell) < list_length(methodNames) - 1)
 				appendStringInfo(es->str, ", ");
 		}
-		appendStringInfo(es->str, ")");
 
 		if (groupInfo->maxMemorySpaceUsed > 0)
 		{
@@ -2740,7 +2755,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
 			const char *spaceTypeName;
 
 			spaceTypeName = tuplesort_space_type_name(SORT_SPACE_TYPE_MEMORY);
-			appendStringInfo(es->str, " %s: %ldkB (avg), %ldkB (max)",
+			appendStringInfo(es->str, " %s: avg=%ldkB peak=%ldkB",
 							 spaceTypeName, avgSpace,
 							 groupInfo->maxMemorySpaceUsed);
 		}
@@ -2755,7 +2770,7 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
 			/* Add a semicolon separator only if memory stats were printed. */
 			if (groupInfo->maxMemorySpaceUsed > 0)
 				appendStringInfo(es->str, ";");
-			appendStringInfo(es->str, " %s: %ldkB (avg), %ldkB (max)",
+			appendStringInfo(es->str, " %s: avg=%ldkB peak=%ldkB",
 							 spaceTypeName, avgSpace,
 							 groupInfo->maxDiskSpaceUsed);
 		}
@@ -2764,7 +2779,6 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
 	}
 	else
 	{
-		List	   *methodNames = NIL;
 		StringInfoData groupName;
 
 		initStringInfo(&groupName);
@@ -2772,12 +2786,6 @@ show_incremental_sort_group_info(IncrementalSortGroupInfo *groupInfo,
 		ExplainOpenGroup("Incremental Sort Groups", groupName.data, true, es);
 		ExplainPropertyInteger("Group Count", NULL, groupInfo->groupCount, es);
 
-		foreach(methodCell, groupInfo->sortMethods)
-		{
-			const char *sortMethodName = tuplesort_method_name(methodCell->int_value);
-
-			methodNames = lappend(methodNames, unconstify(char *, sortMethodName));
-		}
 		ExplainPropertyList("Sort Methods Used", methodNames, es);
 
 		if (groupInfo->maxMemorySpaceUsed > 0)
@@ -2834,15 +2842,14 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
 	if (!(es->analyze && fullsortGroupInfo->groupCount > 0))
 		return;
 
-	show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort", es);
+	show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort", true, es);
 	prefixsortGroupInfo = &incrsortstate->incsort_info.prefixsortGroupInfo;
 	if (prefixsortGroupInfo->groupCount > 0)
-		show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", es);
+		show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", true, es);
 
 	if (incrsortstate->shared_info != NULL)
 	{
 		int			n;
-		bool		opened_group = false;
 
 		for (n = 0; n < incrsortstate->shared_info->num_workers; n++)
 		{
@@ -2860,20 +2867,18 @@ show_incremental_sort_info(IncrementalSortState *incrsortstate,
 				prefixsortGroupInfo->groupCount == 0)
 				continue;
 
-			if (!opened_group)
-			{
-				ExplainOpenGroup("Workers", "Workers", false, es);
-				opened_group = true;
-			}
+			if (es->workers_state)
+				ExplainOpenWorker(n, es);
 
 			if (fullsortGroupInfo->groupCount > 0)
-				show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort", es);
+				show_incremental_sort_group_info(fullsortGroupInfo, "Full-sort",
+												 es->workers_state == NULL, es);
 			if (prefixsortGroupInfo->groupCount > 0)
-				show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", es);
-		}
+				show_incremental_sort_group_info(prefixsortGroupInfo, "Presorted", true, es);
 
-		if (opened_group)
-			ExplainCloseGroup("Workers", "Workers", false, es);
+			if (es->workers_state)
+				ExplainCloseWorker(n, es);
+		}
 	}
 }
 
diff --git a/src/backend/executor/nodeIncrementalSort.c b/src/backend/executor/nodeIncrementalSort.c
index 9fe93d5979..6c683538ff 100644
--- a/src/backend/executor/nodeIncrementalSort.c
+++ b/src/backend/executor/nodeIncrementalSort.c
@@ -85,6 +85,30 @@
 #include "utils/lsyscache.h"
 #include "utils/tuplesort.h"
 
+/*
+ * We need to store the instrumentation information in either local node's sort
+ * info or, for a parallel worker process, in the shared info (this avoids
+ * having to additionally memcpy the info from local memory to shared memory
+ * at each instrumentation call). This macro expands to choose the proper sort
+ * state and group info.
+ *
+ * Arguments:
+ * - node: type IncrementalSortState *
+ * - groupName: the token fullsort or prefixsort
+ */
+#define INSTRUMENT_SORT_GROUP(node, groupName) \
+	if (node->ss.ps.instrument != NULL) \
+	{ \
+		if (node->shared_info && node->am_worker) \
+		{ \
+			Assert(IsParallelWorker()); \
+			Assert(ParallelWorkerNumber <= node->shared_info->num_workers); \
+			instrumentSortedGroup(&node->shared_info->sinfo[ParallelWorkerNumber].groupName##GroupInfo, node->groupName##_state); \
+		} else { \
+			instrumentSortedGroup(&node->incsort_info.groupName##GroupInfo, node->groupName##_state); \
+		} \
+	}
+
 /* ----------------------------------------------------------------
  * instrumentSortedGroup
  *
@@ -94,12 +118,10 @@
  * ----------------------------------------------------------------
  */
 static void
-instrumentSortedGroup(PlanState *pstate, IncrementalSortGroupInfo *groupInfo,
+instrumentSortedGroup(IncrementalSortGroupInfo *groupInfo,
 					  Tuplesortstate *sortState)
 {
-	IncrementalSortState *node = castNode(IncrementalSortState, pstate);
 	TuplesortInstrumentation sort_instr;
-
 	groupInfo->groupCount++;
 
 	tuplesort_get_stats(sortState, &sort_instr);
@@ -122,19 +144,7 @@ instrumentSortedGroup(PlanState *pstate, IncrementalSortGroupInfo *groupInfo,
 	}
 
 	/* Track each sort method we've used. */
-	if (!list_member_int(groupInfo->sortMethods, sort_instr.sortMethod))
-		groupInfo->sortMethods = lappend_int(groupInfo->sortMethods,
-											 sort_instr.sortMethod);
-
-	/* Record shared stats if we're a parallel worker. */
-	if (node->shared_info && node->am_worker)
-	{
-		Assert(IsParallelWorker());
-		Assert(ParallelWorkerNumber <= node->shared_info->num_workers);
-
-		memcpy(&node->shared_info->sinfo[ParallelWorkerNumber],
-			   &node->incsort_info, sizeof(IncrementalSortInfo));
-	}
+	groupInfo->sortMethods |= sort_instr.sortMethod;
 }
 
 /* ----------------------------------------------------------------
@@ -434,10 +444,7 @@ switchToPresortedPrefixMode(PlanState *pstate)
 		SO1_printf("Sorting presorted prefix tuplesort with %ld tuples\n", nTuples);
 		tuplesort_performsort(node->prefixsort_state);
 
-		if (pstate->instrument != NULL)
-			instrumentSortedGroup(pstate,
-								  &node->incsort_info.prefixsortGroupInfo,
-								  node->prefixsort_state);
+		INSTRUMENT_SORT_GROUP(node, prefixsort)
 
 		if (node->bounded)
 		{
@@ -695,10 +702,7 @@ ExecIncrementalSort(PlanState *pstate)
 				SO1_printf("Sorting fullsort with %ld tuples\n", nTuples);
 				tuplesort_performsort(fullsort_state);
 
-				if (pstate->instrument != NULL)
-					instrumentSortedGroup(pstate,
-										  &node->incsort_info.fullsortGroupInfo,
-										  fullsort_state);
+				INSTRUMENT_SORT_GROUP(node, fullsort)
 
 				SO_printf("Setting execution_status to INCSORT_READFULLSORT (final tuple)\n");
 				node->execution_status = INCSORT_READFULLSORT;
@@ -779,10 +783,7 @@ ExecIncrementalSort(PlanState *pstate)
 							   nTuples);
 					tuplesort_performsort(fullsort_state);
 
-					if (pstate->instrument != NULL)
-						instrumentSortedGroup(pstate,
-											  &node->incsort_info.fullsortGroupInfo,
-											  fullsort_state);
+					INSTRUMENT_SORT_GROUP(node, fullsort)
 
 					SO_printf("Setting execution_status to INCSORT_READFULLSORT (found end of group)\n");
 					node->execution_status = INCSORT_READFULLSORT;
@@ -821,10 +822,8 @@ ExecIncrementalSort(PlanState *pstate)
 				 */
 				SO1_printf("Sorting fullsort tuplesort with %ld tuples\n", nTuples);
 				tuplesort_performsort(fullsort_state);
-				if (pstate->instrument != NULL)
-					instrumentSortedGroup(pstate,
-										  &node->incsort_info.fullsortGroupInfo,
-										  fullsort_state);
+
+				INSTRUMENT_SORT_GROUP(node, fullsort)
 
 				/*
 				 * If the full sort tuplesort happened to switch into top-n
@@ -937,10 +936,7 @@ ExecIncrementalSort(PlanState *pstate)
 		SO1_printf("Sorting presorted prefix tuplesort with >= %ld tuples\n", nTuples);
 		tuplesort_performsort(node->prefixsort_state);
 
-		if (pstate->instrument != NULL)
-			instrumentSortedGroup(pstate,
-								  &node->incsort_info.prefixsortGroupInfo,
-								  node->prefixsort_state);
+		INSTRUMENT_SORT_GROUP(node, prefixsort)
 
 		SO_printf("Setting execution_status to INCSORT_READPREFIXSORT (found end of group)\n");
 		node->execution_status = INCSORT_READPREFIXSORT;
@@ -1026,13 +1022,13 @@ ExecInitIncrementalSort(IncrementalSort *node, EState *estate, int eflags)
 		fullsortGroupInfo->totalDiskSpaceUsed = 0;
 		fullsortGroupInfo->maxMemorySpaceUsed = 0;
 		fullsortGroupInfo->totalMemorySpaceUsed = 0;
-		fullsortGroupInfo->sortMethods = NIL;
+		fullsortGroupInfo->sortMethods = 0;
 		prefixsortGroupInfo->groupCount = 0;
 		prefixsortGroupInfo->maxDiskSpaceUsed = 0;
 		prefixsortGroupInfo->totalDiskSpaceUsed = 0;
 		prefixsortGroupInfo->maxMemorySpaceUsed = 0;
 		prefixsortGroupInfo->totalMemorySpaceUsed = 0;
-		prefixsortGroupInfo->sortMethods = NIL;
+		prefixsortGroupInfo->sortMethods = 0;
 	}
 
 	/*
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 6127ab5912..8d1b944472 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2034,7 +2034,7 @@ typedef struct IncrementalSortGroupInfo
 	long		totalDiskSpaceUsed;
 	long		maxMemorySpaceUsed;
 	long		totalMemorySpaceUsed;
-	List	   *sortMethods;
+	Size		sortMethods; /* bitmask of TuplesortMethod */
 } IncrementalSortGroupInfo;
 
 typedef struct IncrementalSortInfo
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index 0e9ab4e586..96e970339c 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -61,14 +61,17 @@ typedef struct SortCoordinateData *SortCoordinate;
  * Data structures for reporting sort statistics.  Note that
  * TuplesortInstrumentation can't contain any pointers because we
  * sometimes put it in shared memory.
+ *
+ * TuplesortMethod is used in a bitmask in Increment Sort's shared memory
+ * instrumentation so needs to have each value be a separate bit.
  */
 typedef enum
 {
 	SORT_TYPE_STILL_IN_PROGRESS = 0,
-	SORT_TYPE_TOP_N_HEAPSORT,
-	SORT_TYPE_QUICKSORT,
-	SORT_TYPE_EXTERNAL_SORT,
-	SORT_TYPE_EXTERNAL_MERGE
+	SORT_TYPE_TOP_N_HEAPSORT = 2,
+	SORT_TYPE_QUICKSORT = 4,
+	SORT_TYPE_EXTERNAL_SORT = 8,
+	SORT_TYPE_EXTERNAL_MERGE = 16
 } TuplesortMethod;
 
 typedef enum
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index ebb8412237..9a9cb9f28c 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -531,13 +531,13 @@ select * from (select * from t order by a) s order by a, b limit 55;
 
 -- Test EXPLAIN ANALYZE with only a fullsort group.
 select explain_analyze_without_memory('select * from (select * from t order by a) s order by a, b limit 55');
-                                 explain_analyze_without_memory                                  
--------------------------------------------------------------------------------------------------
+                                 explain_analyze_without_memory                                 
+------------------------------------------------------------------------------------------------
  Limit (actual rows=55 loops=1)
    ->  Incremental Sort (actual rows=55 loops=1)
          Sort Key: t.a, t.b
          Presorted Key: t.a
-         Full-sort Groups: 2 (Methods: quicksort, top-N heapsort) Memory: NNkB (avg), NNkB (max)
+         Full-sort Groups: 2 Sort Methods: top-N heapsort, quicksort Memory: avg=NNkB peak=NNkB
          ->  Sort (actual rows=100 loops=1)
                Sort Key: t.a
                Sort Method: quicksort  Memory: NNkB
@@ -563,8 +563,8 @@ select jsonb_pretty(explain_analyze_inc_sort_nodes_without_memory('select * from
          "Full-sort Groups": {                   +
              "Group Count": 2,                   +
              "Sort Methods Used": [              +
-                 "quicksort",                    +
-                 "top-N heapsort"                +
+                 "top-N heapsort",               +
+                 "quicksort"                     +
              ],                                  +
              "Sort Space Memory": {              +
                  "Average Sort Space Used": "NN",+
@@ -705,14 +705,14 @@ select * from t left join (select * from (select * from t order by a) v order by
 rollback;
 -- Test EXPLAIN ANALYZE with both fullsort and presorted groups.
 select explain_analyze_without_memory('select * from (select * from t order by a) s order by a, b limit 70');
-                         explain_analyze_without_memory                          
----------------------------------------------------------------------------------
+                        explain_analyze_without_memory                         
+-------------------------------------------------------------------------------
  Limit (actual rows=70 loops=1)
    ->  Incremental Sort (actual rows=70 loops=1)
          Sort Key: t.a, t.b
          Presorted Key: t.a
-         Full-sort Groups: 1 (Methods: quicksort) Memory: NNkB (avg), NNkB (max)
-         Presorted Groups: 5 (Methods: quicksort) Memory: NNkB (avg), NNkB (max)
+         Full-sort Groups: 1 Sort Method: quicksort Memory: avg=NNkB peak=NNkB
+         Presorted Groups: 5 Sort Method: quicksort Memory: avg=NNkB peak=NNkB
          ->  Sort (actual rows=100 loops=1)
                Sort Key: t.a
                Sort Method: quicksort  Memory: NNkB
-- 
2.21.1

