On Thu, Sep 8, 2016 at 2:19 PM, Peter Geoghegan <p...@heroku.com> wrote: >> Peter, looking at your "displace" patch in this light, I think >> tuplesort_heap_root_displace() and tuplesort_heap_delete_top() (as I'm >> calling it now), should share a common subroutine. Displace replaces the top >> node with the new node passed as argument, and then calls the subroutine to >> push it down to the right place. Delete_top replaces the top node with the >> last value in the heap, and then calls the subroutine. Or perhaps delete_top >> should remove the last value from the heap, and then call displace() with >> it, to re-insert it. > > I can see the value in that, but I'd still rather not. The code reuse > win isn't all that large, and having a separate > tuplesort_heap_root_displace() routine allows us to explain what's > going on for merging, to assert what we're able to assert with tape > numbers vs. heap position, and to lose the HEAPCOMPARE()/checkIndex > cruft that the existing routines need (if only barely) to support > replacement selection. I would be surprised if with a very tight inner > loop like this, HEAPCOMPARE() has an appreciable overhead (maybe it > increases pipeline stalls).
BTW, regarding this, since I read in some other thread that it was ok to use static inline functions, I believe the compiler is smart enough to optimize out the constant true/false in checkIndex for inlined calls, so that may be viable (on modern compilers). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers