Re: [PATCH] Incremental sort (was: PoC: Partial sort)
James Coleman <jtc331@gmail.com>
On Thu, Jul 4, 2019 at 10:46 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > > On Thu, Jul 04, 2019 at 09:29:49AM -0400, James Coleman wrote: > >On Tue, Jun 25, 2019 at 7:22 PM Tomas Vondra > ><tomas.vondra@2ndquadrant.com> wrote: > >> > >> On Tue, Jun 25, 2019 at 04:53:40PM -0400, James Coleman wrote: > >> > > >> >Unrelated: if you or someone else you know that's more familiar with > >> >the parallel code, I'd be interested in their looking at the patch at > >> >some point, because I have a suspicion it might not be operating in > >... > >> So I've looked into that, and the reason seems fairly simple - when > >> generating the Gather Merge paths, we only look at paths that are in > >> partial_pathlist. See generate_gather_paths(). > >> > >> And we only have sequential + index paths in partial_pathlist, not > >> incremental sort paths. > >> > >> IMHO we can do two things: > >> > >> 1) modify generate_gather_paths to also consider incremental sort for > >> each sorted path, similarly to what create_ordered_paths does > >> > >> 2) modify build_index_paths to also generate an incremental sort path > >> for each index path > >> > >> IMHO (1) is the right choice here, because it automatically does the > >> trick for all other types of ordered paths, not just index scans. So, > >> something like the attached patch, which gives me plans like this: > >... > >> But I'm not going to claim those are total fixes, it's the minimum I > >> needed to do to make this particular type of plan work. > > > >Thanks for looking into this! > > > >I intended to apply this to my most recent version of the patch (just > >sent a few minutes ago), but when I apply it I noticed that the > >partition_aggregate regression tests have several of these failures: > > > >ERROR: could not find pathkey item to sort > > > >I haven't had time to look into the cause yet, so I decided to wait > >until the next patch revision. > > > > FWIW I don't claim the patch I shared is complete and/or 100% correct. > It was more an illustration of the issue and the smallest patch to make > a particular query work. The test failures are a consequence of that. > > I'll try looking into the failures over the next couple of days, but I > can't promise anything. Yep, I understand, I just wanted to note that it was still an outstanding item and give a quick update on why so. Anything you can look at is much appreciated. James Coleman
Commits
-
Further adjustments to Hashagg EXPLAIN ANALYZE output
- 40efbf8706cd 14.0 cited
-
Rework EXPLAIN format for incremental sort
- 6a918c3ac8a6 13.0 landed
-
Fix typos and improve incremental sort comments
- 1a40d37a9faf 13.0 landed
-
Stabilize incremental_sort tests
- cea09246e578 13.0 landed
-
Minor improvements in Incremental Sort explain
- d22782a5392f 13.0 landed
-
Consider Incremental Sort paths at additional places
- ba3e76cc571e 13.0 landed
-
Fix representation of SORT_TYPE_STILL_IN_PROGRESS.
- c7654f6a3779 13.0 landed
-
Fix failures in incremental_sort due to number of workers
- 23ba3b5ee278 13.0 landed
-
Fix show_incremental_sort_info with force_parallel_mode
- 7d6d82a52493 13.0 landed
-
Implement Incremental Sort
- d2d8a229bc58 13.0 landed
-
Fix handling of "Subplans Removed" field in EXPLAIN output.
- 7d91b604d9b5 13.0 cited
-
Fix EXPLAIN (SETTINGS) to follow policy about when to print empty fields.
- 3ec20c7091e9 13.0 cited
-
Ensure plpgsql result tuples have the right composite type marking.
- 5683b34956b4 13.0 cited
-
Propagate sort instrumentation from workers back to leader.
- bf11e7ee2e36 11.0 cited
-
Make new regression test case parallel-safe, and improve its output.
- 1177ab1dabf7 11.0 cited
-
Push limit through subqueries to underlying sort, where possible.
- 1f6d515a67ec 11.0 cited
-
Fix inappropriate printing of never-measured times in EXPLAIN.
- 4b234fd8bf21 9.6.0 cited
-
Fix some infelicities in EXPLAIN output for parallel query plans.
- 8ebb69f85445 9.6.0 cited