Re: [HACKERS] [PATCH] Incremental sort

Hi all,

This is the other Alexander K. speaking.


On 06.04.2018 20:26, Tomas Vondra wrote:
> I personally am OK with reducing the scope of the patch like this. It's
> still beneficial for the common ORDER BY + LIMIT case, which is good. I
> don't think it may negatively affect other cases (at least I can't think
> of any).

I think we can reduce it even further. Just try incremental sort along 
with full sort over the cheapest path in create_ordered_paths, and don't 
touch anything else. This is a very minimal and a probably safe start, 
and then we can continue working on other, more complex cases. In the 
attached patch I tried to do this. We probably should also remove 
changes in make_sort() and create a separate function 
make_incremental_sort() for it, but I'm done for today.


> 1) pathkeys_useful_for_ordering() still uses enable_incrementalsort,
> which I think is a bad idea. I've complained about it in my review on
> 31/3, and I don't see any explanation why this is a good idea.

Removed.

> 2) Likewise, I've suggested that the claim about abbreviated keys in
> nodeIncrementalsort.c is dubious. No response, and the XXX comment was
> instead merged into the patch:
>
>   * XXX The claim about abbreviated keys seems rather dubious, IMHO.

Not sure about that, maybe just use abbreviated keys for the first 
version? Later we can research this more closely and maybe start 
deciding whether to use abbrev on planning stage.

> 3) There is a comment at cost_merge_append, despite there being no
> relevant changes in that function. Misplaced comment?

Removed.

> 4) It's not clear to me why INITIAL_MEMTUPSIZE is defined the way it is.
> There needs to be a comment - the intent seems to be making it large
> enough to exceed ALLOCSET_SEPARATE_THRESHOLD, but it's not quite clear
> why that's a good idea.

Not sure myself, let's ask the other Alexander.


> 5) I do get this warning when building the code:
>
> costsize.c: In function ‘cost_incremental_sort’:
> costsize.c:1812:2: warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>    List    *presortedExprs = NIL;
>    ^~~~
>
> 6) The comment at cost_incremental_sort talks about cost_sort_internal,
> but it's cost_sort_tuplesort I guess.

Fixed.

> 7) The new code in create_sort_path is somewhat ugly, I guess. It's
> correct, but it really needs to be made easier to comprehend. I might
> have time to look into that tomorrow, but I can't promise that.

Removed this code altogether, now the costs are compared by add_path as 
usual.

> Attached is a diff highlighting some of those places, and couple of
> minor code formatting fixes.

Applied.

Also some other changes from me:

Remove extra blank lines
label_sort_with_costsize shouldn't have to deal with IncrementalSort 
plans, because they are only created from corresponding Path nodes.
Reword a comment in ExecSupportsBackwardsScan.
Clarify cost calculations.
enable_incrementalsort is checked at path level, we don't have to check 
it again at plan level.
enable_sort should act as a cost-based soft disable for both incremental 
and normal sort.

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