On Thu, Jan 26, 2012 at 4:09 PM, Peter Geoghegan <pe...@2ndquadrant.com> wrote: > 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?
Apparently not. The sort is too short to register in the trace_sort output. I just get: LOG: begin tuple sort: nkeys = 1, workMem = 1024, randomAccess = f LOG: performsort starting: CPU 0.00s/0.00u sec elapsed 0.00 sec LOG: performsort done: CPU 0.00s/0.00u sec elapsed 0.00 sec LOG: internal sort ended, 853 KB used: CPU 0.00s/0.00u sec elapsed 0.00 sec >> 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. Possibly. At a minimum it keeps the door open. >> 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. I was hoping so... >> 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. Cool. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
