Re: Missing HashAgg EXPLAIN ANALYZE details for parallel plans

On Fri, 19 Jun 2020 at 16:06, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Jun 19, 2020 at 03:03:41PM +1200, David Rowley wrote:
> > On Fri, 19 Jun 2020 at 14:20, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > Please be sure to use two spaces between each field !
> > >
> > > See earlier discussions (and commits referenced by the Opened Items page).
> > > https://www.postgresql.org/message-id/20200402054120.GC14618@telsasoft.com
> > > https://www.postgresql.org/message-id/20200407042521.GH2228%40telsasoft.com
> >
> > Thanks. I wasn't aware of that conversion.
>
> To be clear, there wasn't a "conversion".

Sorry, I meant to write "conversation".

>
> Some minor nitpicks now that we've dealt with the important issue of
> whitespace:
>
> +               bool    gotone = false;
>
> => Would you consider calling that "found" ?

I'll consider it.  I didn't really invent the name. There's plenty of
other places that use that name for the same thing.  I think of
"found" as more often used when we're looking for something and need
to record if we found it or not.  That's not really happening here. I
just want to record if we've added a property yet.

> +               int n;
> +
> +               for (n = 0; n < aggstate->shared_info->num_workers; n++)
>
> => Maybe use a C99 declaration ?

Maybe.  It'll certainly save a couple of lines.

> +                               if (hash_batches_used > 0)
> +                               {
> +                                       ExplainPropertyInteger("Disk Usage", "kB", hash_disk_used,
> +                                                                                  es);
> +                                       ExplainPropertyInteger("HashAgg Batches", NULL,
> +                                                                                  hash_batches_used, es);
> +                               }
>
> => Shouldn't those *always* be shown in nontext format ?  I realize that's not
> a change new to your patch, and maybe should be a separate commit.

Perhaps a separate commit. I don't want to overload the debate with
too many things. I'd rather push forward with just the originally
proposed change since nobody has shown any objection for it.

> +   size = offsetof(SharedAggInfo, sinstrument)
> +       + pcxt->nworkers * sizeof(AggregateInstrumentation);
>
> => There's a couple places where I'd prefer to see "+" at the end of the
> preceding line (but YMMV).

I pretty much just copied the whole of that code from nodeSort.c. I'm
more inclined to just keep it as similar to that as possible. However,
if pgindent decides otherwise, then I'll go with that. I imagine it
won't move it though as that code has already been through indentation
a few times before.

Thanks for the review.

David