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
