RE: Confusing comment for function ExecParallelEstimate

Sorry, Last mail forget to CC the mailing list.

Now the comment is confusing, Maybe someone should correct it.

Here is a simple patch, What do you think ?

With Regards,
Wu Fei

-----Original Message-----
From: Amit Kapila [mailto:amit.kapila16@gmail.com] 
Sent: Wednesday, June 05, 2019 7:18 PM
To: Wu, Fei/吴 非 <wufei.fnst@cn.fujitsu.com>
Cc: pgsql-hackers@postgresql.org
Subject: 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/execut
> or/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
SharedPlanStateInstrumentationwith Instrumentation in the comment.
 

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