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
Date: Thu, 18 Jun 2020 23:06:24 -0500
Message-ID: <20200619040624.GA17995@telsasoft.com> (raw)
In-Reply-To: <CAApHDvqN5Y7qtqMfNyFYVcFnskjdYknTf1PEZpwDG1uxXRAOaw@mail.gmail.com>
References: <CAApHDvpEKbfZa18mM1TD7qV6PG+w97pwCWq5tVD0dX7e11gRJw@mail.gmail.com>
<20200618134514.GF17995@telsasoft.com>
<CAApHDvpsd7Un=ZGdLPDA4QFEEdtXSoiwTRG8Y3zr5JEFpMfhOA@mail.gmail.com>
<20200619022001.GY17995@telsasoft.com>
<CAApHDvqN5Y7qtqMfNyFYVcFnskjdYknTf1PEZpwDG1uxXRAOaw@mail.gmail.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, dgrowleyml@gmail.com, pgsql@j-davis.com
Subject: Re: Missing HashAgg EXPLAIN ANALYZE details for parallel plans
In-Reply-To: <20200619040624.GA17995@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox