Re: explain HashAggregate to report bucket and memory stats

Hi,

On 2020-03-06 15:33:10 -0600, Justin Pryzby wrote:
> On Fri, Mar 06, 2020 at 09:58:59AM -0800, Andres Freund wrote:
> > > + ExplainIndentText(es);
> > > + appendStringInfo(es->str,
> > > + "Buckets: %ld (originally %ld)",
> > > + inst->nbuckets,
> > > + inst->nbuckets_original);
> >
> > I'm not sure I like the alternative output formats here. All the other
> > fields are separated with a comma, but the original size is in
> > parens. I'd probably just format it as "Buckets: %lld " and then add
> > ", Original Buckets: %lld" when differing.
>
> It's done that way for consistency with hashJoin in show_hash_info().

Fair. I don't like it, but it's not this patch's fault.

> > > diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
> > > index f457b5b150..b173b32cab 100644
> > > --- a/src/test/regress/expected/aggregates.out
> > > +++ b/src/test/regress/expected/aggregates.out
> > > @@ -517,10 +517,11 @@ order by 1, 2;
> > > -> HashAggregate
> > > Output: s2.s2, sum((s1.s1 + s2.s2))
> > > Group Key: s2.s2
> > > + Buckets: 4
> > > -> Function Scan on pg_catalog.generate_series s2
> > > Output: s2.s2
> > > Function Call: generate_series(1, 3)
> > > -(14 rows)
> > > +(15 rows)
> >
> > These tests probably won't be portable. The number of hash buckets
> > calculated will e.g. depend onthe size of the contained elements. And
> > that'll e.g. will depend on whether pointers are 4 or 8 bytes.
>
> I was aware and afraid of that. Previously, I added this output only to
> "explain analyze", and (as an quick, interim implementation) changed various
> tests to use analyze, and memory only shown in "verbose" mode. But as Tomas
> pointed out, that's consistent with what's done elsewhere.
>
> So is the solution to show stats only during explain ANALYZE ?
>
> Or ... I have a patch to create a new explain(MACHINE) option to allow more
> stable output, by avoiding Memory/Disk. That doesn't attempt to make all
> "explain analyze" output stable - there's other issues, I think mostly related
> to parallel workers (see 4ea03f3f, 13e8b2ee). But does allow retiring
> explain_sq_limit and explain_parallel_sort_stats. I'm including my patch to
> show what I mean, but I didn't enable it for hashtable "Buckets:". I guess in
> either case, the tests shouldn't be included.

Yea, there's been recent discussion about an argument like that. See
e.g.
https://www.postgresql.org/message-id/18494.1580079189%40sss.pgh.pa.us

Greetings,

Andres Freund