Re: Duplicate Workers entries in some EXPLAIN plans - Mailing list pgsql-hackers

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