Re: [HACKERS] [PATCH] Incremental sort

Hi Alexander,

I took a quick look at the patch. Some things I fixed myself in the
attached patch v.21. Here is the summary:

Typo in compare_fractional_path_costs() should be fixed as a separate patch.
Remove unused function estimate_pathkeys_groups.
Extra MemoryContextReset before tuplesort_end() shouldn't be a big deal,
so we don't have to add a parameter to tuplesoft_free().
Add comment to maincontext declaration.
Fix typo in INITIAL_MEMTUPSIZE.
Remove trailing whitespace.

Some other things I found:

In tuplesort_reset:
if (state->memtupsize < INITIAL_MEMTUPSIZE)
    <reallocate memtuples to INITIAL_MEMTUPSIZE>
I'd add a comment explaining when and why we have to do this. Also maybe
a comment to other allocations of memtuples in tuplesort_begin() and
mergeruns(), explaining why it is reallocated and why in maincontext.

In tuplesort_updatemax:
    /* XXX */
    if (spaceUsedOnDisk > state->maxSpaceOnDisk ||
        (spaceUsedOnDisk == state->maxSpaceOnDisk && spaceUsed >
state->maxSpace))
XXX. Also comparing bools with '>' looks confusing to me.

We should add a comment on top of tuplesort.c, explaining that we now
have a faster way to sort multiple batches of data using the same sort
conditions.

The name 'main context' sounds somewhat vague. Maybe 'top context'? Not
sure.

In ExecSupportBackwardsScan:
        case T_IncrementalSort:
            return false;
This separate case looks useless, I'd either add a comment explaining
why it can't scan backwards, or just return false by default.

That's all I have for today; tomorrow I'll continue with reviewing the
planner part of the patch.

--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company