On Thu, Jan 19, 2012 at 1:47 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> Thoughts?
I generated some random data using this query:
create table nodups (g) as select (g%10)*1000+g/10 from
generate_series(1,10000) g;
And then used pgbench to repeatedly sort it using this query:
select * from nodups order by g offset 10001;
The patch came out about 28% faster than master. Admittedly, that's
with no client overhead, but still: not bad.
I don't like the API you've designed, though: as you have it,
PrepareSortSupportFromOrderingOp does this after calling the sort
support function:
+ ssup->usable_compar = ResolveComparatorProper(sortFunction);
I think it would be better to simply have the sort support functions
set usable_compar themselves. That would allow any user-defined
functions that happen to have the same binary representation and
comparison rules as one of the types for which we supply a custom
qsort() to use initialize it to still make use of the optimization.
There's no real reason to have a separate switch to decide how to
initialize that field: the sort support trampoline already does that,
and I don't see any reason to introduce a second way of doing the same
thing.
I am also a little unhappy that we have to duplicate code the fastcmp
functions from nbtcompare.c in builtins.h. Wouldn't it make more
sense to declare those functions as inline in nbtcompare.c, and then
call the qsort-generating macro from that file?
There were a couple of comment updates in tuplesort.c that looked
independent from the reset of the patch, so I've committed those
separately. I also committed your change to downgrade the
belt-and-suspenders check for self-comparison to an assert, with some
rewording of your proposed comment.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company