From: | James Coleman <jtc331(at)gmail(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Cc: | 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>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andreas Karlsson <andreas(at)proxel(dot)se> |
Subject: | Re: [PATCH] Incremental sort (was: PoC: Partial sort) |
Date: | 2020-03-30 02:16:53 |
Message-ID: | CAAaqYe__vp0QQHgUfiVe+NrcMpECfQgZO8iu2CEwVOcBgA+S0Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Mar 29, 2020 at 9:44 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)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?
> 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.
> My TODO for this patch is this:
>
> - review the costing (I think the estimates are OK, but I recall I
> haven't been entirely happy with how it's broken into functions.)
>
> - review the tuplesort changes (the memory contexts etc.)
>
> - do more testing of performance impact on planning
Sounds good.
Thanks,
James
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-03-30 02:17:36 | Re: Berserk Autovacuum (let's save next Mandrill) |
Previous Message | Tom Lane | 2020-03-30 02:10:46 | Re: snapper vs. HEAD |