Antonin Houska <a...@cybertec.at> wrote:
> Alexander Korotkov <a.korot...@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
