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: Michael Paquier <michael@paquier.xyz>, Rafia Sabih <rafia.pghackers@gmail.com>, Peter Geoghegan <pg@bowt.ie>, Simon Riggs <simon@2ndquadrant.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>, Alvaro Herrera <alvherre@2ndquadrant.com>, andreas@proxel.se
Date: 2020-03-23T02:17:41Z
Lists: pgsql-hackers 
On Fri, Mar 20, 2020 at 8:56 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> Hi,
>
> I've looked at v38 but it seems it's a bit broken by some recent explain
> changes (mostly missing type in declarations). Attached is v39 fixing
> those issues, and including a bunch of fixes based on a review - most of
> the changes is in comments, so I've instead kept them in separate "fix"
> patches after each part.
>
> In general I'm mostly happy with the current shape of the patch, and
> unless there are some objections I'd like to get some of it committed
> sometime next week.
>
> I've done a fair amount of testing with various queries, and the plan
> changes seem pretty sensible. I'm still not entirely sure whether to be
> a bit conservative and only tweak the first patch adding incremental
> sort to extra places, or commit both.
>
> The main thing I still have on my plate is assessment of how much more
> expensive can the planning due to increased number of paths we
> generate/keep (due to considering extra pathkeys). I haven't seen any
> significant slowdowns, but I plan to look at some extreme cases (many
> similar and applicable indexes etc.).

I'm currently incorporating all of the fixes you proposed into the
main patch series, as well as doing a thorough read-through of the
current state of the patch. I'm hoping to reply tomorrow with:

- Fix patches of my own to clean up and add additional comments.
- Catalog all of the current open questions (XXX, etc.) in the patch
to more easily discuss them in the mailing list.

One question I have while I work on that: I've noticed some confusion
in the patch as to whether we should refer to the node below the
incremental sort node in the plan tree (i.e., the node we get tuples
from) as the inner node or the outer node. Intuitively I'd expect to
call it the inner node, but the original patch referred to it
frequently as the outer node. The outerPlanState/innerPlanState macro
comments don't offer a lot of clarification though they're "to avoid
confusion" about right/left inner/outer. I suppose if the
outerPlanState macro is working here the correct term should be outer?

Thanks,
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.