Re: Explain buffers wrong counter with parallel plans

Hi,

I'm not an expert in the area of the code, but here's a review anyway. I
did not read through the entire thread.


I think we should try to get this fixed soon, to make some progress
towards release-ability. Or just declare it to be entirely unrelated to
the release, and remove it from the open items list; not an unreasonable
choice either. This has been an open item for quite a while...


> diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
> index 60aaa822b7e..ac22bedf0e2 100644
> --- a/src/backend/executor/execParallel.c
> +++ b/src/backend/executor/execParallel.c
> @@ -979,9 +979,6 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
>  	/* Report workers' query for monitoring purposes */
>  	pgstat_report_activity(STATE_RUNNING, debug_query_string);
>  
> -	/* Prepare to track buffer usage during query execution. */
> -	InstrStartParallelQuery();
> -
>  	/* Attach to the dynamic shared memory area. */
>  	area_space = shm_toc_lookup(toc, PARALLEL_KEY_DSA, false);
>  	area = dsa_attach_in_place(area_space, seg);
> @@ -993,6 +990,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
>  	queryDesc->planstate->state->es_query_dsa = area;
>  	ExecParallelInitializeWorker(queryDesc->planstate, toc);
>  
> +	/* Prepare to track buffer usage during query execution. */
> +	InstrStartParallelQuery();
> +
>  	/* Run the plan */
>  	ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);

Isn't this an independent change?  And one with potentially negative
side effects? I think there's some arguments for changing this (and some
against), but I think it's a bad idea to do so in the same patch.


> diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
> index 36d2914249c..a0d49ec0fba 100644
> --- a/src/backend/executor/execProcnode.c
> +++ b/src/backend/executor/execProcnode.c
> @@ -737,6 +737,13 @@ ExecShutdownNode(PlanState *node)
>  
>  	planstate_tree_walker(node, ExecShutdownNode, NULL);
>  
> +	/*
> +	 * Allow instrumentation to count stats collected during shutdown for
> +	 * nodes that are executed atleast once.
> +	 */
> +	if (node->instrument && node->instrument->running)
> +		InstrStartNode(node->instrument);
> +
>  	switch (nodeTag(node))
>  	{
>  		case T_GatherState:
> @@ -755,5 +762,12 @@ ExecShutdownNode(PlanState *node)
>  			break;
>  	}
>  
> +	/*
> +	 * Allow instrumentation to count stats collected during shutdown for
> +	 * nodes that are executed atleast once.
> +	 */
> +	if (node->instrument && node->instrument->running)
> +		InstrStopNode(node->instrument, 0);
> +
>  	return false;
>  }

Duplicating the comment doesn't seem like a good idea. Just say
something like "/* see comment for InstrStartNode above */".


> diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
> index ac5a2ff0e60..cf1851e235f 100644
> --- a/src/backend/executor/nodeLimit.c
> +++ b/src/backend/executor/nodeLimit.c
> @@ -134,6 +134,8 @@ ExecLimit(PlanState *pstate)
>  					node->position - node->offset >= node->count)
>  				{
>  					node->lstate = LIMIT_WINDOWEND;
> +					/* Allow nodes to release or shut down resources. */
> +					(void) ExecShutdownNode(outerPlan);
>  					return NULL;
>  				}
>  

Um, is this actually correct?  What if somebody rewinds afterwards?
That won't happen for parallel query currently, but architecturally I
don't think we can do so unconditionally?


Greetings,

Andres Freund