On Sun, Mar 29, 2020 at 9:44 PM Tomas Vondra <tomas.von...@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? > 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
