Horton

Re: [HACKERS] [PATCH] Incremental sort

Antonin Houska <ah@cybertec.at>

View thread View on postgr.es
From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@postgresql.org
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Date: 2017-11-15T16:20:33Z
Lists: pgsql-hackers 
Antonin Houska <ah@cybertec.at> wrote:

> Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
> 
> > Patch rebased to current master is attached. I'm going to improve my testing script and post new results. 
> 
> I wanted to review this patch but incremental-sort-8.patch fails to apply. Can
> you please rebase it again?

I could find the matching HEAD quite easily (9b6cb46), so following are my comments:

* cost_sort()

** "presorted_keys" missing in the prologue

** when called from label_sort_with_costsize(), 0 is passed for
   "presorted_keys". However label_sort_with_costsize() can sometimes be
   called on an IncrementalSort, in which case there are some "presorted
   keys". See create_mergejoin_plan() for example. (IIUC this should only
   make EXPLAIN inaccurate, but should not cause incorrect decisions.)


** instead of

if (!enable_incrementalsort)
   presorted_keys = false;

you probably meant

if (!enable_incrementalsort)
   presorted_keys = 0;


** instead of

/* Extract presorted keys as list of expressions */
foreach(l, pathkeys)
{
	PathKey *key = (PathKey *)lfirst(l);
	EquivalenceMember *member = (EquivalenceMember *)
		lfirst(list_head(key->pk_eclass->ec_members));

you can use linitial():

/* Extract presorted keys as list of expressions */
foreach(l, pathkeys)
{
	PathKey *key = (PathKey *)lfirst(l);
	EquivalenceMember *member = (EquivalenceMember *)
		linitial(key->pk_eclass->ec_members);


* get_cheapest_fractional_path_for_pathkeys()

The prologue says "... at least partially satisfies the given pathkeys ..."
but I see no change in the function code. In particular the use of
pathkeys_contained_in() does not allow for any kind of partial sorting.


* pathkeys_useful_for_ordering()

Extra whitespace following the comment opening string "/*":

/* 
 * When incremental sort is disabled, pathkeys are useful only when they


* make_sort_from_pathkeys() - the "skipCols" argument should be mentioned in
  the prologue.


* create_sort_plan()

I noticed that pathkeys_common() is called, but the value of n_common_pathkeys
should already be in the path's "skipCols" field if the underlying path is
actually IncrementalSortPath.


* create_unique_plan() does not seem to make use of the incremental
  sort. Shouldn't it do?


* nodeIncrementalSort.c

** These comments seem to contain typos:

"Incremental sort algorithm would sort by xfollowing groups, which have ..."

"Interate while skip cols are same as in saved tuple"

** (This is rather a pedantic comment) I think prepareSkipCols() should be
   defined in front of cmpSortSkipCols().

** the MIN_GROUP_SIZE constant deserves a comment.


* ExecIncrementalSort()

** if (node->tuplesortstate == NULL)

If both branches contain the expression

     node->groupsCount++;

I suggest it to be moved outside the "if" construct.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

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.