Re: Duplicate Workers entries in some EXPLAIN plans

gkokolatos@pm.me

From: Georgios Kokolatos <gkokolatos@pm.me>
To: pgsql-hackers@lists.postgresql.org
Cc: Maciek Sakrejda <m.sakrejda@gmail.com>
Date: 2020-01-22T17:36:20Z
Lists: pgsql-hackers
>> +       int num_workers = planstate->worker_instrument->num_workers;
>> +       int n;
>> +       worker_strs = (StringInfo *) palloc0(num_workers * sizeof(StringInfo));
>> +       for (n = 0; n < num_workers; n++) {
>>
>> I think C99 would be better here. Also no parenthesis needed.
>
>
> Pardon my C illiteracy, but what am I doing that's not valid C99 here?

My bad, I should have been more clear. I meant that it is preferable to use
the C99 standard which calls for declaring variables in the scope that you
need them. In this case, 'n' is needed only in the for loop, so something like

for (int n = 0; n < num_workers; n++) 

is preferable. To be clear, your code was perfectly valid. It was only the
style I was referring to.

>> +       for (n = 0; n < w->num_workers; ++n)
>>
>> I think C99 would be better here.
>
>
> And here (if it's not the same problem)?

Exactly the same as above. 

>>     int         indent;         /* current indentation level */
>>     List       *grouping_stack; /* format-specific grouping state */
>> +   bool        print_workers;  /* whether current node has worker metadata */
>>
>> Hmm.. commit <b925a00f4ef> introduced `hide_workers` in the struct. Having both
>> names in the struct so far apart even seems a bit confusing and sloppy. Do you
>> think it would be possible to combine or rename?
>
>
> I noticed that. I was thinking about combining them, but
> "hide_workers" seems to be about "pretend there is no worker output
> even if there is" and "print_workers" is "keep track of whether or not
> there is worker output to print". Maybe I'll rename to
> "has_worker_output"?

The rename sounds a bit better in my humble opinion. Thanks.

Commits

  1. Clean up EXPLAIN's handling of per-worker details.