Re: Duplicate Workers entries in some EXPLAIN plans

On Wed, Jan 22, 2020 at 9:37 AM Georgios Kokolatos <gkokolatos(at)pm(dot)me> wrote:
> 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.

Ah, I see. I think I got that from ExplainPrintSettings. I've
corrected my usage--thanks for pointing it out. I appreciate the
effort to maintain a consistent style.

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

Also, reviewing my code again, I noticed that when I moved the general
worker output earlier, I missed part of the merge conflict: I had
replaced

- /* Show worker detail */
- if (es->analyze && es->verbose && !es->hide_workers &&
- planstate->worker_instrument)

with

+ /* Prepare worker general execution details */
+ if (es->analyze && es->verbose && planstate->worker_instrument)

which ignores the es->hide_workers flag (it did not fail the tests,
but the intent is pretty clear). I've corrected this in the current
patch.

I also noticed that we can now handle worker buffer output more
consistently across TEXT and structured formats, so I made that small
change too:

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 140d0be426..b23b015594 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1401,8 +1401,6 @@ ExplainNode(PlanState *planstate, List *ancestors,
appendStringInfo(es->str,

"actual rows=%.0f loops=%.0f\n",

rows, nloops);
- if (es->buffers)
- show_buffer_usage(es,
&instrument->bufusage);
}
else
{
@@ -1951,7 +1949,7 @@ ExplainNode(PlanState *planstate, List *ancestors,

/* Prepare worker buffer usage */
if (es->buffers && es->analyze && es->verbose && !es->hide_workers
- && planstate->worker_instrument && es->format !=
EXPLAIN_FORMAT_TEXT)
+ && planstate->worker_instrument)
{
WorkerInstrumentation *w = planstate->worker_instrument;
int n;
diff --git a/src/test/regress/expected/explain.out
b/src/test/regress/expected/explain.out
index 8034a4e0db..a4eed3067f 100644
--- a/src/test/regress/expected/explain.out
+++ b/src/test/regress/expected/explain.out
@@ -103,8 +103,8 @@ $$, 'verbose', 'analyze', 'buffers', 'timing off',
'costs off', 'summary off'),
Sort Key: (ROW("*VALUES*".column1)) +
Buffers: shared hit=114 +
Worker 0: actual rows=2 loops=1 +
- Buffers: shared hit=114 +
Sort Method: xxx +
+ Buffers: shared hit=114 +
-> Values Scan on "*VALUES*" (actual rows=2 loops=1) +
Output: "*VALUES*".column1, ROW("*VALUES*".column1)+
Worker 0: actual rows=2 loops=1 +

I think the "producing plan output for a worker" process is easier to
reason about now, and while it changes TEXT format worker output
order, the other changes in this patch are more drastic so this
probably does not matter.

I've also addressed the other feedback above, and reworded a couple of
comments slightly.