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

On Tue, Apr 07, 2020 at 08:40:30AM -0400, James Coleman wrote:
>On Tue, Apr 7, 2020 at 12:25 AM Justin Pryzby  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.
>

I'm OK with changing the format if there's a consensus. The current
format seemed better to me, but I'm not particularly attached to it.

>> 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?
>

Yeah, I don't think there was a clear consensus :-(

>> 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.
>

Will fix.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services