Date: Thu, 18 Jun 2020 23:06:24 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Jeff Davis <pgsql@j-davis.com>
Subject: Re: Missing HashAgg EXPLAIN ANALYZE details for parallel plans
Message-ID: <20200619040624.GA17995@telsasoft.com>
References: 
 <CAApHDvpEKbfZa18mM1TD7qV6PG+w97pwCWq5tVD0dX7e11gRJw@mail.gmail.com>
 <20200618134514.GF17995@telsasoft.com>
 <CAApHDvpsd7Un=ZGdLPDA4QFEEdtXSoiwTRG8Y3zr5JEFpMfhOA@mail.gmail.com>
 <20200619022001.GY17995@telsasoft.com>
 <CAApHDvqN5Y7qtqMfNyFYVcFnskjdYknTf1PEZpwDG1uxXRAOaw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvqN5Y7qtqMfNyFYVcFnskjdYknTf1PEZpwDG1uxXRAOaw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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".  There were (and are still) different
formats (which everyone agrees isn't ideal) used by "explain(BUFFERS)" vs Sort
and Hash.  The new incr sort changed from output that looked like Buffers (one
space, and equals) to output that looked like Sort/Hashjoin (two spaces and
colons).  And the new explain(WAL) originally used a hybrid (which, on my
request, used two spaces), but it was changed (back) to use one space, for
consistency with explain(BUFFERS).

Some minor nitpicks now that we've dealt with the important issue of
whitespace:

+               bool    gotone = false;

=> Would you consider calling that "found" ?

I couldn't put my finger on it at first why this felt so important to ask, but
realized that my head was tripping over a variable whose name starts with
"goto", and spending 0.2 seconds trying to figure out what you might have meant
by "goto ne".

+               int n;
+
+               for (n = 0; n < aggstate->shared_info->num_workers; n++)

=> Maybe use a C99 declaration ?

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

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

-- 
Justin