On 26 January 2012 19:45, Robert Haas <robertmh...@gmail.com> wrote: > The patch came out about 28% faster than master. Admittedly, that's > with no client overhead, but still: not bad.
Thanks. There was a 28% reduction in the time it took to execute the query, but there would have also been a larger reduction in the time that the backend held that all of that locally-allocated memory. That might also be worth instrumenting directly, by turning on "trace_sort" - can you report numbers on that, please? > 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. Hmm. You're right. I can't believe that that didn't occur to me. In practice, types that use the SortSupport API are all going to be façades on scalar types anyway, much like date and timestamp, and of those a good proportion will surely have the same comparator representation as the specialisations introduced by this patch. It might be that virtually all third-party types that end up using the API can avail of some specialisation. > 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? Maybe it would, but since the meta-qsort_arg introduced includes partial duplicates of code from tuplesort.c, it kind of felt right to "instantiate" specialisations there. It may be that doing it in nbtcompare.c is the best option available to us. Off the top of my head, I'm pretty sure that that's a good bit less code. > 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. That seems reasonable. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers