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(dot)kapila16(at)gmail(dot)com]
Sent: Wednesday, June 05, 2019 7:18 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 11:27 AM Wu, Fei <wufei(dot)fnst(at)cn(dot)fujitsu(dot)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 SharedPlanStateInstrumentation with Instrumentation in the comment.

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