Re: nodeAgg perf tweak

On Wed, 2004-12-01 at 15:54 -0500, Tom Lane wrote:
> This seems like it might work. Instead of copying the result into the
> aggcontext on every cycle, we could copy it only when we intend to reset
> the working context.

Right.

> This is
> problematic since the source tuple will go away on the next cycle.
> The existing copying code takes care of this case as well as the one
> where the result is in per_tuple_context.

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.

> Digging in the
> list archives, it looks like I neglected to report that failure, but
> I do still have the patch and I attach it here. This was from late
> Dec '03 so it'd be against just-past-7.4 sources. I'd be interested
> to hear how this compares to what you did under your test conditions;
> maybe I was using a bogus test case.

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). You can find the
test data at:

http://www.langille.org/watch_list_elements.sql.tgz

Just load the .sql file, run ANALYZE, and try the queries I posted.

-Neil