Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Tomas Vondra <tomas.vondra@2ndquadrant.com>
On Sun, Sep 15, 2019 at 09:33:33PM -0400, James Coleman wrote: >On Sat, Jul 20, 2019 at 11:25 AM Tomas Vondra ><tomas.vondra@2ndquadrant.com> wrote: >> >> ... >> >> >> >> I think this may be a thinko, as this plan demonstrates - but I'm not >> >> sure about it. I wonder if this might be penalizing some other types of >> >> plans (essentially anything with limit + gather). >> >> >> >> Attached is a WIP patch fixing this by considering both startup and >> >> total cost (by calling compare_path_costs_fuzzily). >> > >> >It seems to me that this is likely a bug, and not just a changed >> >needed for this. Do you think it's better addressed in a separate >> >thread? Or retain it as part of this patch for now (and possibly break >> >it out later)? On the other hand, it's entirely possible that someone >> >more familiar with parallel plan limitations could explain why the >> >above comment holds true. That makes me lean towards asking in a new >> >thread. >> > >> >> Maybe. I think creating a separate thread would be useful, provided we >> manage to demonstrate the issue without an incremental sort. > >I did some more thinking about this, and I can't currently come up >with a way to reproduce this issue outside of this patch. It doesn't >seem reasonable to me to assume that there's anything inherent about >this patch that means it's the only way we can end up with a partial >path with a low startup cost we'd want to prefer. > >Part of me wants to pull it over to a separate thread just to get >additional feedback, but I'm not sure how useful that is given we >don't currently have an example case outside of this patch. > Hmm, I see. While I initially suggested to start a separate thread only if we have example not involving an incremental sort, that's probably not a hard requirement. I think it's fine to start a thead briefly explaining the issue, and pointing to incremental sort thread for actual example. > >One thing to note though: the current patch does not also modify >add_partial_path_precheck which also does not take into account >startup cost, so we probably need to update that for completeness's >sake. > Good point. It does indeed seem to make the same assumption about only comparing total cost before calling add_path_precheck. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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