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

From: Florian Pflug <fgp(at)phlo(dot)org>
To: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Rowley <dgrowleyml(at)gmail(dot)com>, Kevin Grittner <kgrittn(at)ymail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Greg Stark <stark(at)mit(dot)edu>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Negative Transition Aggregate Functions (WIP)
Date: 2014-04-10 21:54:33
Message-ID: A33459B2-0E9C-426B-8F1D-9A61E7B1E286@phlo.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Apr10, 2014, at 21:34 , Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 10 April 2014 19:54, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> So if we go with that terminology, perhaps these names for the
>> new CREATE AGGREGATE parameters:
>>
>> initfunc applies to plain aggregation, mutually exclusive with initcond
>> msfunc (or just mfunc?) forward transition for moving-agg mode
>> mifunc inverse transition for moving-agg mode
>> mstype state datatype for moving-agg mode
>> msspace space estimate for mstype
>> mfinalfunc final function for moving-agg mode
>> minitfunc "firsttrans" for moving-agg mode
>> minitcond mutually exclusive with minitfunc
>
> I think I prefer "mfunc" to "msfunc", but perhaps that's just my
> natural aversion to the "ms" prefix :-)
>
> Also, perhaps "minvfunc" rather than "mifunc" because "i" by itself
> could mean "initial".

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.

I don't think we should be additional that much additional machinery
until it has been conclusively demonstrated that the performance
regression cannot be fixed any other way. Which, quite frankly, I
don't believe. Nothing in int4_avg_accum looks particularly expensive,
and the things that *do* seem to cost something certainly can be made
cheaper. c.f. the patch I just posted.

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? Add yet a another set of
"mergable" transition functions? What about the various combinations
of invertible/non-invertible mergable/non-mergable that could result?
The opportunity cost seems pretty high here...

best regards,
Florian Pflug

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2014-04-10 22:07:28 Re: [PATCH] Negative Transition Aggregate Functions (WIP)
Previous Message Tom Lane 2014-04-10 21:52:26 Re: [PATCH] Negative Transition Aggregate Functions (WIP)