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