Re: [PATCH] Incremental sort (was: PoC: Partial sort)

On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby <pryzby(at)telsasoft(dot)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