Date: Wed, 8 Jul 2020 11:30:21 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
	Jeff Davis <pgsql@j-davis.com>
Subject: Re: Missing HashAgg EXPLAIN ANALYZE details for parallel plans
Message-ID: <20200708163021.GW4107@telsasoft.com>
References: 
 <CAApHDvpEKbfZa18mM1TD7qV6PG+w97pwCWq5tVD0dX7e11gRJw@mail.gmail.com>
 <20200618134514.GF17995@telsasoft.com>
 <CAApHDvpsd7Un=ZGdLPDA4QFEEdtXSoiwTRG8Y3zr5JEFpMfhOA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvpsd7Un=ZGdLPDA4QFEEdtXSoiwTRG8Y3zr5JEFpMfhOA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Fri, Jun 19, 2020 at 02:02:29PM +1200, David Rowley wrote:
> if (es->format == EXPLAIN_FORMAT_TEXT)
> {
> ExplainIndentText(es);
> appendStringInfo(es->str, "Sort Method: %s  %s: %ldkB\n",
...

> As for this patch, I don't think it's unreasonable to have the 3
> possible HashAgg properties on a single line Other people might
> disagree, so here's an example of what the patch changes it to:
> 
> postgres=# explain analyze select a,sum(b) from ab group by a;
>                                                      QUERY PLAN
> --------------------------------------------------------------------------------------------------------------------
>  HashAggregate  (cost=175425.12..194985.79 rows=988 width=12) (actual
> time=1551.920..5281.670 rows=1000 loops=1)
>    Group Key: a
>    Peak Memory Usage: 97 kB Disk Usage: 139760 kB HashAgg Batches: 832
>    ->  Seq Scan on ab  (cost=0.00..72197.00 rows=5005000 width=8) (actual time=0.237..451.228 rows=5005000 loops=1)

I've just noticed another inconsistency:
For "Sort", there's no space before "kB", but your patch (9bdb300d) uses a
space for text mode.

+               appendStringInfo(es->str, "Peak Memory Usage: " INT64_FORMAT " kB",
+                                                memPeakKb);
+
+               if (aggstate->hash_batches_used > 0)
+                       appendStringInfo(es->str, "  Disk Usage: " UINT64_FORMAT " kB  HashAgg Batches: %d",

...

+                               appendStringInfo(es->str, "Peak Memory Usage: " INT64_FORMAT " kB",
+                                                                memPeakKb);
+
+                               if (hash_batches_used > 0)
+                                       appendStringInfo(es->str, "  Disk Usage: " UINT64_FORMAT " kB  HashAgg Batches: %d",

Hopefully there's the final whitespace inconsistency for this release.

-- 
Justin