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