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
structuresin 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.kapila16@gmail.com] 
Sent: Wednesday, June 05, 2019 12:20 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 9:24 AM Wu, Fei <wufei.fnst@cn.fujitsu.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
readingthe 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