Re: explain HashAggregate to report bucket and memory stats

On Sat, Feb 22, 2020 at 10:53:35PM +0100, Tomas Vondra wrote:
> 1) explain.c API
>
> The functions in explain.c (even the static ones) follow the convention
> that the last parameter is (ExplainState *es). I think we should stick
> to this, so the new parameters should be added before it.

I found it weird to have the "constant" arguments at the end rather than at the
beginning. (Also, these don't follow that convention: show_buffer_usage
ExplainSaveGroup ExplainRestoreGroup ExplainOneQuery ExplainPrintJIT).

But done.

> Also, the first show_grouping_sets should be renamed to aggstate to make
> it consistent with the type change.

The prototype wasn't updated - fixed.

> 2) The hash_instrumentation is a bit inconsistent with what we already
> have ..HashTableInstrumentation..

Thanks for thinking of a better name.

> 5) I think the explain for grouping sets need a rething. Teh function
> show_grouping_set_keys was originally meant to print just the keys, but
> now it's also printing the hash table stats. IMO we need a new function
> printing a grouping set info - calling show_grouping_set_keys to print
> the keys, but then also printing the extra hashtable info.

I renamed it, and did the rest in a separate patch for now, since I'm only
partially convinced it's an improvement.

> 6) subplan explain
>
> That is, there's no indication why would this use a hash table, because
> the "hashed subplan" is included only in verbose mode:

Need to think about that..

> Not sure if this is an issue, maybe it's fine. But it's definitely
> strange that we only print memory info in verbose mode - IMHO it's much
> more useful info than the number of buckets etc.

You're right that verbose isn't right for this.

I wrote patches creating new explain options to allow stable output of "explain
analyze", by avoiding Memory/Disk. The only other way to handle it seems to be
to avoid "explain analyze" in regression tests, which is what's in common
practice anyway, so did that instead.

I also fixed wrong output and wrong non-text formatting for grouping sets,
tweaked output for subplan, and broke style rules less often.

--
Justin