Re: WITHIN GROUP patch

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> Retesting with your changes shows that the gap is down to 15% but still
> present:

There's probably some overhead from traversing advance_transition_function
for each row, which your version wouldn't have done. 15% sounds pretty
high for that though, since advance_transition_function doesn't have much
to do when the transition function is non strict and the state value is
pass-by-value (which "internal" is). It's possible that we could do
something to micro-optimize that case.

I also wondered if it was possible to omit rechecking AggCheckCallContext
after the first time through in ordered_set_transition(). It seemed
unsafe to perhaps not have that happen at all, since if the point is to
detect misuse then the mere fact that argument 0 isn't null isn't much
comfort. It strikes me though that now we could test for "first call" by
looking at fcinfo->flinfo->fn_extra, and be pretty sure that we've already
checked the context if that isn't null. Whether this would save anything
noticeable isn't clear though; I didn't see AggCheckCallContext high in
the profile.

> Furthermore, I can't help noticing that the increased complexity has
> now pretty much negated your original arguments for moving so much of
> the work out of nodeAgg.c.

The key reason for that was, and remains, not having the behavior
hard-wired in nodeAgg; I believe that this design permits things to be
accomplished in aggregate implementation functions that would not have
been possible with the original patch. I'm willing to accept some code
growth to have that flexibility.

regards, tom lane