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

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?

> 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