Horton

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

Tomas Vondra <tomas.vondra@2ndquadrant.com>

View thread View on postgr.es
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, James Coleman <jtc331@gmail.com>, 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>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>, Andreas Karlsson <andreas@proxel.se>
Date: 2020-04-06T21:40:13Z
Lists: pgsql-hackers 
On Mon, Apr 06, 2020 at 11:12:32PM +0200, Tomas Vondra wrote:
>On Mon, Apr 06, 2020 at 04:54:38PM -0400, Alvaro Herrera wrote:
>>On 2020-Apr-06, Tom Lane wrote:
>>
>>>Locally, things pass without force_parallel_mode, but turning it on
>>>produces failures that look similar to rhinoceros's (didn't examine
>>>other BF members).
>>
>>FWIW I looked at the eight failures there were about fifteen minutes ago
>>and they were all identical.  I can confirm that, in my laptop, the
>>tests work without that GUC, and fail in exactly that way with it.
>>
>
>Yes, there's a thinko in show_incremental_sort_info() and it returns too
>soon. I'll push a fix in a minute.
>

OK, I've pushed a fix - this should make the buildfarm happy again.

It however seems to me a bit more needs to be done. The fix makes
show_incremental_sort_info closer to show_sort_info, but not entirely
because IncrementalSortState does not have sort_Done flag so it still
depends on (fullsortGroupInfo->groupCount > 0). I haven't noticed that
before, but not having that flag seems a bit weird to me.

It also seems possibly incorrect - we may end up with

   fullsortGroupInfo->groupCount == 0
   prefixsortGroupInfo->groupCount > 0

but we won't print anything.

James, any opinion on this? I'd say we should restore the sort_Done flag
and make it work as in plain Sort. Or some comment explaining why
depending on the counts is OK (assuming it is).


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.