On 09/10/2016 03:22 AM, Claudio Freire wrote:
> Overall, however, I believe the patch is in good shape. Only minor
> form issues need to be changed, the functionality seems both desirable
> and ready.
Pushed this "displace root" patch, with some changes:
* I renamed "tuplesort_heap_siftup()" to "tuplesort_delete_top()". I
realize that this is controversial, per the discussion on the "Is
tuplesort_heap_siftup() a misnomer?" thread. However, now that we have a
new function, "tuplesort_heap_replace_top()", which is exactly the same
algorithm as the "delete_top()" algorithm, calling one of them "siftup"
became just too confusing. If anything, the new "replace_top"
corresponds more closely to Knuth's siftup algorithm; delete-top is a
special case of it. I added a comment on that to replace_top. I hope
everyone can live with this.
* Instead of "root_displace", I used the name "replace_top", and
"delete_top" for the old siftup function. Because we use "top" to refer
to memtuples[0] more commonly than "root", in the existing comments.
* I shared the code between the delete-top and replace-top. Delete-top
now calls the replace-top function, with the last element of the heap.
Both functions have the same signature, i.e. they both take the
checkIndex argument. Peter's patch left that out for the "replace"
function, on performance grounds, but if that's worthwhile, that seems
like a separate optimization. Might be worth benchmarking that
separately, but I didn't want to conflate that with this patch.
* I replaced a few more siftup+insert calls with the new combined
replace-top operation. Because why not.
Thanks for the patch, Peter, and thanks for the review, Claudio!
- Heikki