Horton

Re: [PATCH] Incremental sort (was: PoC: Partial sort)

James Coleman <jtc331@gmail.com>

View thread View on postgr.es
From: James Coleman <jtc331@gmail.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Peter Geoghegan <pg@bowt.ie>, Simon Riggs <simon@2ndquadrant.com>, Rafia Sabih <rafia.pghackers@gmail.com>, Shaun Thomas <shaun.thomas@2ndquadrant.com>, Dmitry Dolgov <9erthalion6@gmail.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Tom Lane <tgl@sss.pgh.pa.us>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Date: 2019-09-16T01:33:33Z
Lists: pgsql-hackers 
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.

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.

James Coleman

Commits

  1. Further adjustments to Hashagg EXPLAIN ANALYZE output

  2. Rework EXPLAIN format for incremental sort

  3. Fix typos and improve incremental sort comments

  4. Stabilize incremental_sort tests

  5. Minor improvements in Incremental Sort explain

  6. Consider Incremental Sort paths at additional places

  7. Fix representation of SORT_TYPE_STILL_IN_PROGRESS.

  8. Fix failures in incremental_sort due to number of workers

  9. Fix show_incremental_sort_info with force_parallel_mode

  10. Implement Incremental Sort

  11. Fix handling of "Subplans Removed" field in EXPLAIN output.

  12. Fix EXPLAIN (SETTINGS) to follow policy about when to print empty fields.

  13. Ensure plpgsql result tuples have the right composite type marking.

  14. Propagate sort instrumentation from workers back to leader.

  15. Make new regression test case parallel-safe, and improve its output.

  16. Push limit through subqueries to underlying sort, where possible.

  17. Fix inappropriate printing of never-measured times in EXPLAIN.

  18. Fix some infelicities in EXPLAIN output for parallel query plans.