Re: [PATCH] Incremental sort (was: PoC: Partial sort)
James Coleman <jtc331@gmail.com>
On Sat, Mar 14, 2020 at 10:55 PM James Coleman <jtc331@gmail.com> wrote: > > On Fri, Mar 13, 2020 at 1:06 PM James Coleman <jtc331@gmail.com> wrote: > > > > On Thu, Mar 12, 2020 at 5:53 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > > > I gave this a very quick look; I don't claim to understand it or > > > anything, but I thought these trivial cleanups worthwhile. The only > > > non-cosmetic thing is changing order of arguments to the SOn_printf() > > > calls in 0008; I think they are contrary to what the comment says. > > > > Yes, I think you're correct (re: 0008). > > > > They all look generally good to me, and are included in the attached > > patch series. > > I just realized something about this (unsure if in Alvaro's or in my > applying that) broke make check pretty decently (3 test files broken, > also much slower, and the incremental sort test returns a lot of > obviously broken results). > > I'll take a look tomorrow and hopefully get a fix (probably will reply > to the more recent subthread's though). This took a bit of manually just excluding changes until I got a red/green set because nothing in the patch set looked all incorrect. But it turns out this change breaks things: - if (tuplesort_gettupleslot(read_sortstate, ScanDirectionIsForward(dir), - false, slot, NULL) || node->finished) + if (node->finished || + tuplesort_gettupleslot(read_sortstate, ScanDirectionIsForward(dir), + false, slot, NULL)) I believe what's happening here is that we need the tuplesort_gettupleslot to set the slot to a NULL tuple (if there aren't any left) before we return the slot, but that returns false, so the node->finished check is to ensure that the first time that method nulls out the slot and returns false we still return the value in slot. Since this isn't obvious, I'll add a comment. I think I'm also going to rename node->finished to be more clear. In this debugging I also noticed that we don't set node->finished back to false in rescan, which I assume is a bug, but I don't really understand a whole lot about rescan. IIRC from some previous discussions rescan exists for things like cursors, where you can move back and forth over the result set. Assuming that's the case, do we need explicit tests for cursors using incremental sort? Is there a good strategy for how much to do there (since I don't want to duplicate every non-cursor functional test). Thanks, James
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