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

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Cc: James Coleman <jtc331(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Shaun Thomas <shaun(dot)thomas(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Andreas Karlsson <andreas(at)proxel(dot)se>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Date: 2020-04-07 04:25:21
Message-ID: 20200407042521.GH2228@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 ?

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

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

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);

--
Justin

Attachment Content-Type Size
v1-0001-comment-typos-and-others-Incremental-Sort.patch text/x-diff 12.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2020-04-07 04:27:46 Re: doc review for parallel vacuum
Previous Message Noah Misch 2020-04-07 04:18:47 Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding