On Thu, Jan 26, 2012 at 4:09 PM, Peter Geoghegan <peter(at)2ndquadrant(dot)com> wrote:
> On 26 January 2012 19:45, Robert Haas <robertmhaas(at)gmail(dot)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
> 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.
The Enterprise PostgreSQL Company
In response to
pgsql-hackers by date
|Next:||From: Dimitri Fontaine||Date: 2012-01-26 22:00:39|
|Subject: Re: Command Triggers|
|Previous:||From: Peter Geoghegan||Date: 2012-01-26 21:09:57|
|Subject: Re: Progress on fast path sorting, btree index creation time|