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