Thanks! I'll fix the brace issues. Re: the other items:
> + 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?
> + /* Prepare worker general execution details */
> + if (es->analyze && es->verbose && planstate->worker_instrument)
> + {
> + WorkerInstrumentation *w = planstate->worker_instrument;
> + int n;
> +
> + for (n = 0; n < w->num_workers; ++n)
>
> I think C99 would be better here.
And here (if it's not the same problem)?
> + ExplainCloseGroup("Workers", "Workers", false, es);
> + // do we have any other cleanup to do?
>
> This comment does not really explain anything. Either remove
> or rephrase. Also C style comments.
Good catch, thanks--I had put this in to remind myself (and reviewers)
about cleanup, but I don't think there's anything else to do, so I'll
just drop it.
> 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"?
> +extern void ExplainOpenWorker(StringInfo worker_str, ExplainState *es);
> +extern void ExplainCloseWorker(ExplainState *es);
> +extern void ExplainFlushWorkers(StringInfo *worker_strs, int num_workers, ExplainState *es);
>
> No need to expose those, is there? I feel there should be static.
Good call, I'll update.
