Try again to fix accumulation of parallel worker instrumentation.

When a Gather or Gather Merge node is started and stopped multiple
times, accumulate instrumentation data only once, at the end, instead
of after each execution, to avoid recording inflated totals.

Commit 778e78ae9fa51e58f41cbdc72b293291d02d8984, the previous attempt
at a fix, instead reset the state after every execution, which worked
for the general instrumentation data but had problems for the additional
instrumentation specific to Sort and Hash nodes.

Report by hubert depesz lubaczewski.  Analysis and fix by Amit Kapila,
following a design proposal from Thomas Munro, with a comment tweak
by me.

Discussion: http://postgr.es/m/20171127175631.GA405@depesz.com

3 files changed, 11 insertions, 45 deletions

diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index 6b6064637b8..02b5aa517b5 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -899,12 +899,8 @@ ExecParallelReInitializeDSM(PlanState *planstate,
 											  pcxt);
 			break;
 		case T_HashState:
-			/* even when not parallel-aware, for EXPLAIN ANALYZE */
-			ExecHashReInitializeDSM((HashState *) planstate, pcxt);
-			break;
 		case T_SortState:
-			/* even when not parallel-aware, for EXPLAIN ANALYZE */
-			ExecSortReInitializeDSM((SortState *) planstate, pcxt);
+			/* these nodes have DSM state, but no reinitialization is required */
 			break;
 
 		default:
@@ -977,7 +973,7 @@ ExecParallelRetrieveInstrumentation(PlanState *planstate,
 
 /*
  * Finish parallel execution.  We wait for parallel workers to finish, and
- * accumulate their buffer usage and instrumentation.
+ * accumulate their buffer usage.
  */
 void
 ExecParallelFinish(ParallelExecutorInfo *pei)
@@ -1023,23 +1019,23 @@ ExecParallelFinish(ParallelExecutorInfo *pei)
 	for (i = 0; i < nworkers; i++)
 		InstrAccumParallelQuery(&pei->buffer_usage[i]);
 
-	/* Finally, accumulate instrumentation, if any. */
-	if (pei->instrumentation)
-		ExecParallelRetrieveInstrumentation(pei->planstate,
-											pei->instrumentation);
-
 	pei->finished = true;
 }
 
 /*
- * Clean up whatever ParallelExecutorInfo resources still exist after
- * ExecParallelFinish.  We separate these routines because someone might
- * want to examine the contents of the DSM after ExecParallelFinish and
- * before calling this routine.
+ * Accumulate instrumentation, and then clean up whatever ParallelExecutorInfo
+ * resources still exist after ExecParallelFinish.  We separate these
+ * routines because someone might want to examine the contents of the DSM
+ * after ExecParallelFinish and before calling this routine.
  */
 void
 ExecParallelCleanup(ParallelExecutorInfo *pei)
 {
+	/* Accumulate instrumentation, if any. */
+	if (pei->instrumentation)
+		ExecParallelRetrieveInstrumentation(pei->planstate,
+											pei->instrumentation);
+
 	/* Free any serialized parameters. */
 	if (DsaPointerIsValid(pei->param_exec))
 	{
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 6fe5d69d558..afd7384e945 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1670,19 +1670,6 @@ ExecHashInitializeDSM(HashState *node, ParallelContext *pcxt)
 }
 
 /*
- * Reset shared state before beginning a fresh scan.
- */
-void
-ExecHashReInitializeDSM(HashState *node, ParallelContext *pcxt)
-{
-	if (node->shared_info != NULL)
-	{
-		memset(node->shared_info->hinstrument, 0,
-			   node->shared_info->num_workers * sizeof(HashInstrumentation));
-	}
-}
-
-/*
  * Locate the DSM space for hash table instrumentation data that we'll write
  * to at shutdown time.
  */
diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c
index 73aa3715e6d..d593378f74f 100644
--- a/src/backend/executor/nodeSort.c
+++ b/src/backend/executor/nodeSort.c
@@ -397,23 +397,6 @@ ExecSortInitializeDSM(SortState *node, ParallelContext *pcxt)
 }
 
 /* ----------------------------------------------------------------
- *		ExecSortReInitializeDSM
- *
- *		Reset shared state before beginning a fresh scan.
- * ----------------------------------------------------------------
- */
-void
-ExecSortReInitializeDSM(SortState *node, ParallelContext *pcxt)
-{
-	/* If there's any instrumentation space, clear it for next time */
-	if (node->shared_info != NULL)
-	{
-		memset(node->shared_info->sinstrument, 0,
-			   node->shared_info->num_workers * sizeof(TuplesortInstrumentation));
-	}
-}
-
-/* ----------------------------------------------------------------
  *		ExecSortInitializeWorker
  *
  *		Attach worker to DSM space for sort statistics.