Hi hackers,

On Fri, 23 Aug 2019 09:30:59 -0400
Andrew Dunstan <andrew.duns...@2ndquadrant.com> wrote:

> On 8/23/19 8:47 AM, Pierre Giraud wrote:
> >       "Plans": [
> >         {
> >           "Node Type": "Sort",
> >           "Parent Relationship": "Outer",
> >           "Parallel Aware": false,
> >           "Actual Startup Time": 1558.638,
> >           "Actual Total Time": 2127.522,
> >           "Actual Rows": 3333333,
> >           "Actual Loops": 3,
> >           "Output": ["c1", "c2"],
> >           "Sort Key": ["t1.c1"],
> >           "Sort Method": "external merge",
> >           "Sort Space Used": 126152,
> >           "Sort Space Type": "Disk",
> >           "Workers": [
> >             {
> >               "Worker Number": 0,
> >               "Sort Method": "external merge",
> >               "Sort Space Used": 73552,
> >               "Sort Space Type": "Disk"
> >             },
> >             {
> >               "Worker Number": 1,
> >               "Sort Method": "external merge",
> >               "Sort Space Used": 73320,
> >               "Sort Space Type": "Disk"
> >             }
> >           ],
> >           "Workers": [
> >             {
> >               "Worker Number": 0,
> >               "Actual Startup Time": 1487.846,
> >               "Actual Total Time": 1996.879,
> >               "Actual Rows": 2692973,
> >               "Actual Loops": 1
> >             },
> >             {
> >               "Worker Number": 1,
> >               "Actual Startup Time": 1468.256,
> >               "Actual Total Time": 2012.744,
> >               "Actual Rows": 2684443,
> >               "Actual Loops": 1
> >             }
> >           ],  
> 
> Yes, that's really awful.
> 
> 
> ...
> 
> 
> > I think that the text format should stay as is.
> >
> > For the JSON format however it would be better in my opinion if
> > "Workers" data is merged. Parsing should not imply anything else than
> > "var myObj = JSON.parse(theJsonString);".
> >
> > What do you think?

Please, find in attachment a patch that merge parallel data together.

Note that duplicated keys are discouraged in json specs, but they are forbidden
in yaml ones. See link in the commit message.

Regards,
>From 37595f7eaf7650672e38aa011a5400f5c94c70bf Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <j...@dalibo.com>
Date: Thu, 5 Dec 2019 17:41:17 +0100
Subject: [PATCH] Merge parallel infos for each worker in explain

Explain output was building two different group for parallel
general infos and parallel sort infos for each workers.

This is fine for text output, but not for json where dupplicate
keys are discouraged, neither in yaml were it is an error according
to the specs: https://yaml.org/spec/1.2/spec.html#id2759572.

Reported-by: Pierre Giraud
Discussion: https://www.postgresql.org/message-id/flat/41ee53a5-a36e-cc8f-1bee-63f6565bb1ee%40dalibo.com
Discussion: https://www.postgresql.org/message-id/flat/941.1571722493%40sss.pgh.pa.us#36db9545b24929d39639f6bd79390a06
---
 src/backend/commands/explain.c | 89 ++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 62fb3434a3..10f985a4cb 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -101,6 +101,7 @@ static void show_sortorder_options(StringInfo buf, Node *sortexpr,
 static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 							 List *ancestors, ExplainState *es);
 static void show_sort_info(SortState *sortstate, ExplainState *es);
+static void show_parallel_sort_info(SortState *sortstate, ExplainState *es, int n);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 								ExplainState *es);
@@ -1904,6 +1905,10 @@ ExplainNode(PlanState *planstate, List *ancestors,
 				es->indent++;
 				if (es->buffers)
 					show_buffer_usage(es, &instrument->bufusage);
+
+				if (nodeTag(plan) == T_Sort)
+					show_parallel_sort_info(castNode(SortState, planstate), es, n);
+
 				es->indent--;
 			}
 			else
@@ -1929,6 +1934,9 @@ ExplainNode(PlanState *planstate, List *ancestors,
 				if (es->buffers)
 					show_buffer_usage(es, &instrument->bufusage);
 
+				if (nodeTag(plan) == T_Sort)
+					show_parallel_sort_info(castNode(SortState, planstate), es, n);
+
 				ExplainCloseGroup("Worker", NULL, true, es);
 			}
 		}
@@ -2565,50 +2573,49 @@ show_sort_info(SortState *sortstate, ExplainState *es)
 			ExplainPropertyText("Sort Space Type", spaceType, es);
 		}
 	}
+}
 
-	if (sortstate->shared_info != NULL)
-	{
-		int			n;
-		bool		opened_group = false;
+/*
+ * If it's EXPLAIN ANALYZE, show tuplesort stats for a parallel sort node
+ */
+static void
+show_parallel_sort_info(SortState *sortstate, ExplainState *es, int n)
+{
+	TuplesortInstrumentation *sinstrument;
+	const char *sortMethod;
+	const char *spaceType;
+	long		spaceUsed;
 
-		for (n = 0; n < sortstate->shared_info->num_workers; n++)
-		{
-			TuplesortInstrumentation *sinstrument;
-			const char *sortMethod;
-			const char *spaceType;
-			long		spaceUsed;
-
-			sinstrument = &sortstate->shared_info->sinstrument[n];
-			if (sinstrument->sortMethod == SORT_TYPE_STILL_IN_PROGRESS)
-				continue;		/* ignore any unfilled slots */
-			sortMethod = tuplesort_method_name(sinstrument->sortMethod);
-			spaceType = tuplesort_space_type_name(sinstrument->spaceType);
-			spaceUsed = sinstrument->spaceUsed;
+	if (!es->analyze)
+		return;
 
-			if (es->format == EXPLAIN_FORMAT_TEXT)
-			{
-				appendStringInfoSpaces(es->str, es->indent * 2);
-				appendStringInfo(es->str,
-								 "Worker %d:  Sort Method: %s  %s: %ldkB\n",
-								 n, sortMethod, spaceType, spaceUsed);
-			}
-			else
-			{
-				if (!opened_group)
-				{
-					ExplainOpenGroup("Workers", "Workers", false, es);
-					opened_group = true;
-				}
-				ExplainOpenGroup("Worker", NULL, true, es);
-				ExplainPropertyInteger("Worker Number", NULL, n, es);
-				ExplainPropertyText("Sort Method", sortMethod, es);
-				ExplainPropertyInteger("Sort Space Used", "kB", spaceUsed, es);
-				ExplainPropertyText("Sort Space Type", spaceType, es);
-				ExplainCloseGroup("Worker", NULL, true, es);
-			}
-		}
-		if (opened_group)
-			ExplainCloseGroup("Workers", "Workers", false, es);
+	if (! sortstate->sort_Done || sortstate->tuplesortstate == NULL)
+		return;
+
+	if (sortstate->shared_info == NULL)
+		return;
+
+	sinstrument = &sortstate->shared_info->sinstrument[n];
+
+	if (sinstrument->sortMethod == SORT_TYPE_STILL_IN_PROGRESS)
+		return;		/* ignore any unfilled slots */
+
+	sortMethod = tuplesort_method_name(sinstrument->sortMethod);
+	spaceType = tuplesort_space_type_name(sinstrument->spaceType);
+	spaceUsed = sinstrument->spaceUsed;
+
+	if (es->format == EXPLAIN_FORMAT_TEXT)
+	{
+		appendStringInfoSpaces(es->str, es->indent * 2);
+		appendStringInfo(es->str,
+						 "Sort Method: %s  %s: %ldkB\n",
+						 sortMethod, spaceType, spaceUsed);
+	}
+	else
+	{
+		ExplainPropertyText("Sort Method", sortMethod, es);
+		ExplainPropertyInteger("Sort Space Used", "kB", spaceUsed, es);
+		ExplainPropertyText("Sort Space Type", spaceType, es);
 	}
 }
 
-- 
2.20.1

Reply via email to