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

On Jan14, 2014, at 17:39 , Florian Pflug <fgp@phlo.org> wrote:
> On Jan14, 2014, at 11:06 , David Rowley <dgrowleyml@gmail.com> wrote:
>> Here's a patch which removes sum(numeric) and changes the documents a little to remove a reference to using
sum(numeric)to workaround the fact that there's no inverse transitions for sum(float). I also made a small change in
theaggregates.sql tests to check that the aggfnoid <= 9999. 
>
> I've looked over the patch, here a few comments.
>
> For STRICT pairs of transfer and inverse transfer functions we should complain if any of them ever return NULL. That
cannever be correct anyway, since a STRICT function cannot undo a non-NULL -> NULL transition. 
>
> The same goes for non-STRICT, at least if we ever want to allow an inverse transfer function to indicate "Sorry,
cannotundo in this case, please rescan". If we allowed NULL as just another state value now, we couldn't easily undo
thatlater, so we'd rob ourselves of the obvious way for the inverse transfer function to indicate this condition to its
caller.
>
> The notnullcount machinery seems to apply to both STRICT and non-STRICT transfer function pairs. Shouldn't that be
constrainedto STRICT transfer function pairs? For non-STRICT pairs, it's up to the transfer functions to deal with NULL
inputshowever they please, no? 

I modified the patch to do this, and ran into a problem. Currently, aggregates with state type "internal" cannot have a
stricttransfer function, even if they behave like they did, i.e. ignore non-NULL inputs. This is because the only
possibleinitial value for state type "internal" is NULL, and it's up to the transfer function to create the state -
usuallyupon seeing the first non-NULL input. Now, currently that isn't a huge problem - the transfer function simply
hasto check for NULL input values itself, and if it's indeed conceptually strict, it just returns in this case. 

With inverse transfer functions, however, each such pair of forward and inverse transfer functions would also need to
duplicatethe NULL-counting logic from nodeWindowAgg.c if it want's to be conceptually strict, i.e. ignore NULL inputs,
butreturn NULL if there are *only* NULL inputs (or no inputs at all). That seems like a lot of duplicated code in the
longrun. 

In essence, what we'd want for strict pairs of forward and inverse transfer functions is for the forward transfer
functionto be strict in all arguments except the state, and the inverse to be strict according to the usual definition.
Wecan't express that property of the forward transfer function within pg_proc, but we don't really have to - a local
hackin nodeWindowAgg.c suffices. So what I'm proposing is: 

We allow the forward transfer function to be non-strict even if the inverse is strict, but only if the initial value is
NULL.In that case we behave as if the forward transfer function was strict, except that upon seeing the first non-NULL
inputwe call it with a NULL state. The return value must still be non-NULL in all cases. 

Thoughts?

best regards,
Florian Pflug