RE: Confusing comment for function ExecParallelEstimate

Thanks for your reply.
From the code below:
(https://github.com/postgres/postgres/blob/REL_10_7/src/backend/executor/execParallel.c)
#######################################################################################
443 /*
444 * Give parallel-aware nodes a chance to add to the estimates, and get a
445 * count of how many PlanState nodes there are.
446 */
447 e.pcxt = pcxt;
448 e.nnodes = 0;
449 ExecParallelEstimate(planstate, &e);
450
451 /* Estimate space for instrumentation, if required. */
452 if (estate->es_instrument)
453 {
454 instrumentation_len =
455 offsetof(SharedExecutorInstrumentation, plan_node_id) +
456 sizeof(int) * e.nnodes;
457 instrumentation_len = MAXALIGN(instrumentation_len);
458 instrument_offset = instrumentation_len;
459 instrumentation_len +=
460 mul_size(sizeof(Instrumentation),
461 mul_size(e.nnodes, nworkers));
462 shm_toc_estimate_chunk(&pcxt->estimator, instrumentation_len);
463 shm_toc_estimate_keys(&pcxt->estimator, 1);

#######################################################################################
It seems that e.nnodes which returns from ExecParallelEstimate(planstate, &e) , determines how much instrumentation structures in DSM(line459~line461).
And e.nnodes also determines the length of SharedExecutorInstrumentation-> plan_node_id(line454~line456).

So, I think here it refers to instrumentation.

SharedExecutorInstrumentation is just likes a master that hold the metadata:
struct SharedExecutorInstrumentation
{
int instrument_options;
int instrument_offset;
int num_workers;
int num_plan_nodes; // this equals to e.nnodes from the source code
int plan_node_id[FLEXIBLE_ARRAY_MEMBER];
/* array of num_plan_nodes * num_workers Instrumentation objects follows */
};

What do you think?

With Regards,
Wu Fei

-----Original Message-----
From: Amit Kapila [mailto:amit(dot)kapila16(at)gmail(dot)com]
Sent: Wednesday, June 05, 2019 12:20 PM
To: Wu, Fei/吴 非 <wufei(dot)fnst(at)cn(dot)fujitsu(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Confusing comment for function ExecParallelEstimate

On Wed, Jun 5, 2019 at 9:24 AM Wu, Fei <wufei(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
>
> Hi, all
>
> Lately I was researching Parallelism of Postgres 10.7(and it is same in all version), and I was confused when reading the comment of function ExecParallelEstimate :
>
> (in src/backend/executor/execParallel.c)
>
> ----------------------------------------------
>
>
>
> * While we're at it, count the number of PlanState nodes in the tree,
> so
>
> * we know how many SharedPlanStateInstrumentation structures we need.
>
> static bool
>
> ExecParallelEstimate(PlanState *planstate, ExecParallelEstimateContext
> *e)
>
> ----------------------------------------------
>
>
>
> The structure SharedPlanStateInstrumentation is not exists at all. And I noticed that the so called “SharedPlanStateInstrumentation”
>
> maybe is the structure instrumentation now, which is used for storing
> information of planstate in parallelism. The function count the
> number
>
> of planState nodes and stored it in ExecParallelEstimateContext->
> nnodes ,then use it to Estimate space for instrumentation structure in
>
> function ExecInitParallelPlan.
>

I think here it refers to SharedExecutorInstrumentation. This structure is used for accumulating per-PlanState instrumentation. So, it is not totally wrong, but I guess we can change it to SharedExecutorInstrumentation to avoid confusion? What do you think?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com