Re: Duplicate Workers entries in some EXPLAIN plans

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

> TEXT format was tricky due to its inconsistencies, but I think I have
> something working reasonably well. I added a simple test for TEXT
> format output as well, using a similar approach as the JSON format

Great!

> test, and liberally regexp_replacing away any volatile output. I
> suppose in theory we could do this for YAML, too, but I think it's
> gross enough not to be worth it, especially given the high similarity
> of all the structured outputs.

Agreed, what is in the patch suffices. Overall great work, a couple of
minor nitpicks if you allow me.

+ /* Prepare per-worker output */
+ if (es->analyze && planstate->worker_instrument) {

Style, parenthesis on its own line.

+ 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.

+ worker_strs[n] = makeStringInfo();
+ }
+ }

@@ -1357,6 +1369,58 @@ ExplainNode(PlanState *planstate, List *ancestors,
ExplainPropertyBool("Parallel Aware", plan->parallel_aware, es);
}

+ /* 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.

+ {
+ Instrumentation *instrument = &w->instrument[n];
+ double nloops = instrument->nloops;

- appendStringInfoSpaces(es->str, es->indent * 2);
- if (n > 0 || !es->hide_workers)
- appendStringInfo(es->str, "Worker %d: ", n);
+ if (indent)
+ {
+ appendStringInfoSpaces(es->str, es->indent * 2);
+ }

Style: No parenthesis needed

- if (opened_group)
- ExplainCloseGroup("Workers", "Workers", false, es);
+ /* Show worker detail */
+ if (planstate->worker_instrument) {
+ ExplainFlushWorkers(worker_strs, planstate->worker_instrument->num_workers, es);
}

Style: No parenthesis needed

+ * just indent once, to add worker info on the next worker line.
+ */
+ if (es->str == es->root_str)
+ {
+ es->indent += es->format == EXPLAIN_FORMAT_TEXT ? 1 : 2;
+ }
+

Style: No parenthesis needed

+ 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.

+ es->print_workers = false;
+}

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?

+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.

Awaiting for answer or resolution of these comments to change the status.

//Georgios