Re: nodeAgg perf tweak

Neil Conway <neilc(at)samurai(dot)com> writes:
> ISTM it would be reasonable to mandate that aggregate authors return one
> of three things from their state transition functions:

> (a) return the previous state value
> (b) return the "next data item" value
> (c) return some other value; if by a pass-by-reference type, allocated
> in CurrentMemoryContext

> In the case of (b), we need to copy it in advance_transition_function()
> to ensure it survives to the next invocation of the state transition
> function, but that should be doable. For both (a) and (c) we can assume
> the value has been allocated in the per-1000-rows-transition-function
> temporary context, and thus we need only copy it when we reset that
> context.

Well, (1) I'm uncomfortable with "mandating" that aggregate functions do
anything special, and (2) I think you lose much of the performance
benefit as soon as you have to distinguish cases (b) and (c). Ideally
you would use MemoryContextContains for this, but that doesn't work
reliably on pointers that point to fields of a tuple.

I like the approach shown in my prototype patch better, because it
doesn't create any backwards-compatibility issues for existing aggregate
functions; instead individual aggregates may be rewritten to avoid
palloc's. (In fact, you can get to a zero-pallocs-per-cycle condition,
rather than reducing from two to one which is the most that the approach
you suggest could save.)

> I can reproduce the performance improvement with the patch you sent (I
> can repost numbers if you like, but your patch and mine resulted in
> about the same performance when I quickly tested it).

Hmph. I'll have to repeat my test and figure out why I failed to see
much benefit. I had sort of abandoned it in disgust after not getting
results, but obviously I shoulda dug deeper.

regards, tom lane