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

On Apr11, 2014, at 17:09 , Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Basically, this comes down to a design judgment call, and my judgment
> is differing from yours. In the absence of opinions from others,
> I'm going to do it my way.

Ok. Are you going to do the necessary changes, or shall I? I'm happy to
leave it to you, but if you lack the time I can try to find some.

>> ... SUM(int4) wouldn't need
>> *any* extra state at all to be invertible, if it weren't for those pesky
>> issues surrounding NULL handling. In fact, an earlier version of the
>> invtrans patch *did* use int4_sum as SUM(int4)'s transfer functions! The
>> only reason this doesn't work nowadays is that Dean didn't like the
>> forward-nonstrict-but-inverse-strict special case that made this work.
>
> Tell me about the special case here again? Should we revisit the issue?

My original coding allows the combination of non-strict forward with strict
reverse transition functions. The combination behaved like a strict aggregate
regarding NULL handling - i.e., neither the forward nor the reverse transition
function received NULL inputs. But if the initial state was NULL, the forward
transition function *would* be called with that NULL state value upon seeing
the first non-NULL input. In the window case, the aggregation machinery would
also take care to reset the state type to it's initial value when it removed
the last non-NULL input from the aggregation state (just like it does for
strict aggregates today). This had two advantages

1) First, it allows strict aggregates to use state type internal. As it
stands now, aggregates with state type internal must implement their
own NULL handling, even if they behave exactly like most standard
aggregates do, namely ignore NULLS and return NULL only if there were
no non-NULL inputs.

2) It allows strict aggregates whose state type and input type aren't
binary coercible to return NULL if all inputs were NULL without any
special coding. As it stands today, this doesn't work without some
kind of counter in the state, because the final function otherwise
won't know if there were non-NULL inputs or not. Aggregates whose state
and input types are binary coercible get around that by setting the
initial value to NULL while *still* being strict, and relying on the
state replacement behaviour of the aggregate machinery.

It, however, also has a few disadvantages

3) It means that one needs to look at the inverse transition function's
strictness setting even if that function is never used.

4) It feels a bit hacky.

(2) is what affects SUM(int4). The only reason it track the number of inputs
is to be able to return NULL instead of 0 if no inputs remain.

One idea to fix (3) and (4) was *explicitly* flagging aggregates as
NULL-handling or NULL-ignoring, and also as what one might call
"weakly strict", i.e. as returning NULL exactly if there were no non-NULL
inputs. There are various variations of that theme possible - one flag for
each behaviour, or simply a single "common behaviour" flag. In the end, I
decided not to pursue that, mostly because the aggregates affected by that
issued turned out to be relatively easy to fix. For the ones with state type
internal, I simply added a counter, and I made SUM(int4) use AVG's transition
function.

I don't feel too strongly either way on this one - I initially implemented the
exception because I noticed that the NULL handling of some aggregates was
broken otherwise, and it seemed simpler to fix this in one place than going
over all the aggregates separately. OTOH, when I wrote the docs, I noticed
how hard it was to describe the behaviour accurately, which made me like it
less and less. And Dean wasn't happy with it at all, so that finally settled it.

best regards,
Florian Pflug