Re: Inlining comparators as a performance optimisation

On Tue, Nov 22, 2011 at 8:09 PM, Peter Geoghegan <peter(at)2ndquadrant(dot)com> wrote:
> I wonder, is it worth qualifying that the "Sort Method" was a
> "quicksort (fast path)" sort within explain analyze output? This is a
> rather large improvement, so It might actually be something that
> people look out for, as it might be tricky to remember the exact
> circumstances under which the optimisation kicks in by the time we're
> done here.

Well, right now the decision as to which mechanism should be used here
gets made in tuplesort_performsort(), which has no good way of
communicating with EXPLAIN anyway. Actually, I think that's a
modularity violation; using the address of comparetup_heap as a flag
value seems quite ugly. How about moving that logic up to
tuplesort_begin_heap() and having it set some state inside the
Tuplesort, maybe based on a flag in the opclass (or would it have to
attach to the individual operator)?

At least on my machine, your latest patch reliably crashes the
regression tests in multiple places.

The following test case also crashes them for me (perhaps for the same
reason the regression tests crash):

create table i4 (a int, b int);
insert into i4 values (4, 1), (2, 1), (0, 1), (null, 1), (-2, 1), (-7,
1), (4, 2), (4, 3), (4, 4);
select * from i4 order by 1, 2;
TRAP: FailedAssertion("!(state->nKeys == 1)", File: "tuplesort.c", Line: 1261);

The formatting of src/include/utils/template_qsort_arg.h is hard to
read. At ts=8, the backslashes line up, but the code doesn't fit in
80 columns. If you set ts=4, then it fits in 80 columns, but the
backslashes don't line up any more, and the variable declarations
don't either. I believe ts=4 is project standard.

I still think it would be a good idea to provide a mechanism to
override heap_comparetup() with a type-specific function. I don't
think that would take much extra code, and then any data type could
get at least that much benefit out of this.

It seems like it could be a good idea to do some
per-assembler-instruction profiling of this code, and perhaps also of
the original code. I'm curious where the time is being spent.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company