On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Mon, Apr 06, 2020 at 09:57:22PM +0200, Tomas Vondra wrote: > > I've pushed the fist part of this patch series - I've reorganized it a > > I scanned through this again post-commit. Find attached some suggestions. > > Shouldn't non-text explain output always show both disk *and* mem, including > zeros ?
Could you give more context on this? Is there a standard to follow? Regular sort nodes only ever report one type, so there's not a good parallel there. > Should "Pre-sorted Groups:" be on a separate line ? > | Full-sort Groups: 1 Sort Method: quicksort Memory: avg=28kB peak=28kB > Pre-sorted Groups: 1 Sort Method: quicksort Memory: avg=30kB peak=30kB I'd originally had that, but Tomas wanted it to be more compact. It's easy to adjust though if the consensus changes on that. > And, should it use two spaces before "Sort Method", "Memory" and "Pre-sorted > Groups"? I think you should maybe do that instead of the "semicolon > separator". I think "two spaces" makes sense, since the units are different, > similar to hash buckets and normal sort node. > > "Buckets: %d Batches: %d Memory Usage: %ldkB\n", > appendStringInfo(es->str, "Sort Method: %s %s: %ldkB\n", > > Note, I made a similar comment regarding two spaces for explain(WAL) here: > https://www.postgresql.org/message-id/20200402054120.GC14618%40telsasoft.com > > And Peter E seemed to dislike that, here: > https://www.postgresql.org/message-id/ef8c966f-e50a-c583-7b1e-85de6f4ca0d3%402ndquadrant.com I read through that subthread, and the ending seemed to be Peter wanting things to be unified. Was there a conclusion beyond that? > Also, you're showing: > ExplainPropertyInteger("Maximum Sort Space Used", "kB", > groupInfo->maxMemorySpaceUsed, es); > > But in show_hash_info() and show_hashagg_info(), and in your own text output, > that's called "Peak": > ExplainPropertyInteger("Peak Memory Usage", "kB", memPeakKb, es); > ExplainPropertyInteger("Peak Memory Usage", "kB", > spacePeakKb, es); Yes, that's a miss and should be fixed. James
