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-27T18:50:52Z
Lists: pgsql-hackers 
On Mon, Sep 16, 2019 at 6:32 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> 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.

I've started a new thread to discuss:
https://www.postgresql.org/message-id/CAAaqYe-5HmM4ih6FWp2RNV9rruunfrFrLhqFXF_nrrNCPy1Zhg%40mail.gmail.com

James

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.