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
