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

On Apr11, 2014, at 00:07 , Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Florian Pflug <fgp@phlo.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