From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> |
Subject: | Re: explain HashAggregate to report bucket and memory stats |
Date: | 2020-02-25 03:35:32 |
Message-ID: | 20200225033532.GX31889@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v5-0001-explain-to-show-tuplehash-bucket-and-memory-stats.patch | text/x-diff | 75.1 KB |
v5-0002-refactor-show_grouping_set_keys.patch | text/x-diff | 2.9 KB |
v5-0003-Gross-hack-to-put-hash-stats-of-subplans-in-the-r.patch | text/x-diff | 5.4 KB |
v5-0004-implement-hash-stats-for-bitmapHeapScan.patch | text/x-diff | 6.4 KB |
v5-0005-Refactor-for-consistency-symmetry.patch | text/x-diff | 14.7 KB |
v5-0006-TupleHashTable.entrysize-was-unused-except-for-in.patch | text/x-diff | 1.6 KB |
v5-0007-Update-comment-obsolete-since-69c3936a.patch | text/x-diff | 884 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2020-02-25 03:43:15 | Re: Unicode escapes with any backend encoding |
Previous Message | Michael Paquier | 2020-02-25 02:33:54 | Re: client-side fsync() error handling |