Re: Confusing comment for function ExecParallelEstimate

On Wed, Jun 5, 2019 at 11:27 AM Wu, Fei <wufei.fnst@cn.fujitsu.com> wrote:
>
> 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.
>

Right.  I think the way it is mentioned
(SharedPlanStateInstrumentation structures ..) in the comment can
confuse readers.  We can replace SharedPlanStateInstrumentation with
Instrumentation in the comment.

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