Re: [PATCH] Negative Transition Aggregate Functions (WIP)

On Apr11, 2014, at 00:07 , Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> I still think you're getting ahead of yourselves here. The number of
>> aggregates which benefit from this is tiny SUM(int2,int4) and maybe
>> BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
>> for the others, the state type is already pass-by-ref.
>
> That argument is reasonable for the initfunc idea, but it doesn't apply
> otherwise.

Why not? AFAICS, the increase in cost comes from going from an
by-value to a by-reference state type. Once you're using a by-refence
type, you already pay the overhead of the additional dereferences, and
for calling AggCheckCallContext() or some equivalent.

>> Another reason I'm so opposed to this is that inverse transition
>> functions might not be the last kind of transition functions we ever
>> add. For example, if we ever get ROLLUP/CUBE, we might want to have
>> a mergefunc which takes two aggregation states and combines them
>> into one. What do we do if we add those?
>
> Make more pg_aggregate columns. What exactly do you think is either
> easier or harder about such cases? Also, maybe I'm misremembering
> the spec, but ROLLUP/CUBE wouldn't apply to window functions would
> they? So if your argument is based on the assumption that these
> features need to combine, I'm not sure it's true.

Well, it was just an example - there might be other future extensions
which *do* need to combine. And we've been known to go beyond the spec
sometimes...

> Furthermore, I do not buy the argument that if we hack hard enough,
> we can make the performance cost of forcing the sfunc to do double duty
> negligible. In the first place, that argument is unsupported by much
> evidence, and in the second place, it will certainly cost us complexity
> to make the performance issue go away. Instead we can just design the
> problem out, for nothing that I see as a serious drawback.

My argument is that is costs us more complexity to duplicate everything
for the invertible case, *and* the result seems less flexible - not
from the POV of aggregate implementations, but from the POV of future
extensions.

As for evidence - have you looked at the patch I posted? I'd be very
interested to know if it removes the performance differences you saw.

best regards,
Florian Pflug