Re: [HACKERS] [PATCH] Incremental sort

Antonin Houska  wrote:

> Alexander Korotkov  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