Re: Progress on fast path sorting, btree index creation time

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
>> 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