From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Rafia Sabih <rafia.pghackers@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Simon Riggs <simon@2ndquadrant.com>
Cc: Shaun Thomas <shaun.thomas@2ndquadrant.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Cc: Andreas Karlsson <andreas@proxel.se>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Date: Mon, 30 Mar 2020 14:24:26 +0200
Message-ID: <20200330122426.wjdfpqiatv4ybui7@development> (raw)
In-Reply-To: <CAAaqYe__vp0QQHgUfiVe+NrcMpECfQgZO8iu2CEwVOcBgA+S0Q@mail.gmail.com>
References: <20200328025830.6v6ogkseohakc32q@development>
<CAAaqYe-Jhp-QEC2-pwg61aF0vbGR=gEW657cnXxcqUJvdvBaQA@mail.gmail.com>
<20200328213050.byyeuozylnxv2bhf@development>
<20200328225922.ljnw54bfg44vi6ib@development>
<CAAaqYe_P9sJ4ACuaHRU-+pq0HAg76ei052BsUfuWjA9EwSkGMw@mail.gmail.com>
<20200329031437.yvhogo25wdijwpqd@development>
<CAAaqYe-rLqNBeCFq0Ruw4RUs5h1xbO2B1BGnjLq0k5Sw3a2gTg@mail.gmail.com>
<CAAaqYe-M==BwbC47LeMM2HcUGHqCdynNMMyncrBa7t0AGzZGWQ@mail.gmail.com>
<20200330014442.chqnwfmelm3ejsgr@development>
<CAAaqYe__vp0QQHgUfiVe+NrcMpECfQgZO8iu2CEwVOcBgA+S0Q@mail.gmail.com>
On Sun, Mar 29, 2020 at 10:16:53PM -0400, James Coleman wrote:
>On Sun, Mar 29, 2020 at 9:44 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> Hi,
>>
>> Attached is a slightly reorganized patch series. I've merged the fixes
>> into the appropriate matches, and I've also combined the two patches
>> adding incremental sort paths to additional places in planner.
>>
>> A couple more comments:
>>
>>
>> 1) I think the GUC documentation in src/sgml/config.sgml is a bit too
>> detailed, compared to the other enable_* GUCs. I wonder if there's a
>> better place where to move the details. What about adding some examples
>> and explanation to perform.sgml?
>
>I'll take a look at that and include in a patch series tomorrow.
>
>> 2) Looking at the explain output, the verbose mode looks like this:
>>
>> test=# explain (verbose, analyze) select a from t order by a, b, c;
>> QUERY PLAN
>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>> Gather Merge (cost=66.31..816072.71 rows=8333226 width=24) (actual time=4.787..20092.555 rows=10000000 loops=1)
>> Output: a, b, c
>> Workers Planned: 2
>> Workers Launched: 2
>> -> Incremental Sort (cost=66.28..729200.36 rows=4166613 width=24) (actual time=1.308..14021.575 rows=3333333 loops=3)
>> Output: a, b, c
>> Sort Key: t.a, t.b, t.c
>> Presorted Key: t.a, t.b
>> Full-sort Groups: 4169 Sort Method: quicksort Memory: avg=30kB peak=30kB
>> Presorted Groups: 4144 Sort Method: quicksort Memory: avg=128kB peak=138kB
>> Worker 0: actual time=0.766..16122.368 rows=3841573 loops=1
>> Full-sort Groups: 6871 Sort Method: quicksort Memory: avg=30kB peak=30kB
>> Presorted Groups: 6823 Sort Method: quicksort Memory: avg=132kB peak=141kB
>> Worker 1: actual time=1.986..16189.831 rows=3845490 loops=1
>> Full-sort Groups: 6874 Sort Method: quicksort Memory: avg=30kB peak=30kB
>> Presorted Groups: 6847 Sort Method: quicksort Memory: avg=130kB peak=139kB
>> -> Parallel Index Scan using t_a_b_idx on public.t (cost=0.43..382365.92 rows=4166613 width=24) (actual time=0.040..9808.449 rows=3333333 loops=3)
>> Output: a, b, c
>> Worker 0: actual time=0.048..11275.178 rows=3841573 loops=1
>> Worker 1: actual time=0.041..11314.133 rows=3845490 loops=1
>> Planning Time: 0.166 ms
>> Execution Time: 25135.029 ms
>> (22 rows)
>>
>> There seems to be missing indentation for the first line of worker info.
>
>Working on that too.
>
>> I'm still not quite convinced we should be printing two lines - I know
>> you mentioned the lines might be too long, but see how long the other
>> lines may get ...
>
>All right, I give in :)
>
>Do you think non-workers (both the leader and non-parallel plans)
>should also move to one line?
>
I think we should use the same formatting for both cases, so yes.
FWIW I forgot to mention I tweaked the INSTRUMENT_SORT_GROUP macro a
bit, by moving the if condition in it. That makes the calls easier.
>> 3) I see the new nodes (plan state, ...) have "presortedCols" which does
>> not indicate it's a "number of". I think we usually prefix names of such
>> fields "n" or "num". What about "nPresortedCols"? (Nitpicking, I know.)
>
>I can fix this too.
>
>Also I noticed a few compiler warnings I'll fixup in tomorrow's reply.
>
OK
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, jtc331@gmail.com, alvherre@2ndquadrant.com, michael@paquier.xyz, rafia.pghackers@gmail.com, pg@bowt.ie, simon@2ndquadrant.com, shaun.thomas@2ndquadrant.com, 9erthalion6@gmail.com, a.korotkov@postgrespro.ru, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, andreas@proxel.se
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
In-Reply-To: <20200330122426.wjdfpqiatv4ybui7@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox