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

First, I've go the feeling that I should somehow update the commitfest app,
but I don't really know in which way. Should I put myself in as a reviewer,
or as a second author? Or neither? Suggestions welcome...

On Jan17, 2014, at 23:34 , David Rowley <dgrowleyml@gmail.com> wrote:
> The test turned out to become:
>     if (state->expectedScale == -1)
>         state->expectedScale = X.dscale;
>     else if (state->expectedScale != X.dscale)
>         state->expectedScale = -2;
>
> In do_numeric_accum, then when we do the inverse I just check if
> expectedScale < 0 then return NULL.

Ok, so this will rescan if and only if the dscales of all inputs match.
I still that's overly pessimistic - we've only got a problem when we
removed the input with the largest dscale, no? So my suggestion would be
 state->maxDScale = MAX(X.dscale, state->maxDScale);

in do_numeric_accum, and in the inverse
 if (state->maxDScane == X.dscale)   return PG_RETURN_NULL;

I'd think that this avoids more restarts without about the same effort,
but I haven't tried this though, so maybe I'm missing something.

> I'm not set on it, and I'm willing to try the lazy zeroing of the scale
> tracker array, but I'm just not quite sure what extra real use cases it
> covers that the one above does not. Perhaps my way won't perform inverse
> transitions if the query did sum(numericCol / 10) or so.

Dunno how many people SUM over numerics with different dscales. Easily
possible that it's few enough to not be worth fussing over.

> create table num (num numeric(10,2) not null);
> insert into num (num) select * from generate_series(1,20000);
> select sum(num) over(order by num rows between current row and unbounded following) from num; -- 124ms
> select sum(num / 10) over(order by num rows between current row and unbounded following) from num; -- 254ms
> select sum(num / 1) over(order by num rows between current row and unbounded following) from num; -- 108156.917 ms
>
> The divide by 1 case is slow because of that weird 20 trailing zero
> instead of 16 when dividing a numeric by 1 and that causes the inverse
> transition function to return NULL because the scale changes.
>
> I've not tested an unpatched version yet to see how that divide by 1 query
> performs on that but I'll get to that soon.

So without the patch, all three queries should perform simiarly, i.e. take
about 10 seconds, right? If so, the improvement is fantastic!

> I'm thinking that the idea about detecting the numeric range with floating
> point types and performing an inverse transition providing the range has
> not gone beyond some defined danger zone is not material for this patch...
> I think it would be not a great deal of work to code, but the testing involved
> would probably make this patch not possible for 9.4

Yeah, I never imagined that this would happen for 9.4.

> The latest version of the patch is attached.

OK, there are a few more comments

* build_aggregate_fnexprs() should allow NULL to be passed for invtransfn_oid, I think. I don't quite like that we
constructthat even for plain aggregates, and avoiding requires just an additional if. 

* Don't we need to check for volatile function in the filter expression too?

* As it stands, I don't think intXand_inv and intXor_inv are worth it, since the case where they return non-NULL is
awefullyslim (only for inputs containing only 1 respectively only zeros). We should either track the number of zeros
andones per bit, which would allow us to always perform inverse transitions, or just drop them.  

* Quite a few of the inverse transition functions are marked strict, yet contain code to handle NULL inputs. You can
justremove that code - the system makes sure that strict functions never receive NULL arguments. Affected are, AFAICS
numeric_accum_inv,numeric_avg_accum_inv, int2_accum_inv,  int4_accum_inv, int8_accum_inv, int8_avg_accum_inv,
int2_sum_inv,int4_sum_inv, int8_sum_inv. Not sure that list is exhaustive... 

* For any of the new inverse transition functions, I'd be inclined to just elog() if they're called directly and not as
anaggregate. In particular those which check for that anyway, plus the *smaller_inv and *larger_inv ones. I don't see
whyanyone would ever want to call these directly - and if we elog() now, we can easy change these functions later,
becauseno external code can depend on them. E.g., maybe someone wants to keep the top N elements in the MIN/MAX
aggregatesone day... 

* The number of new regression tests seems a bit excessive. I don't think there really a policy what to test and what
not,but in this case I think it suffices if we test the basic machinery, plus the more complex functions. But e.g. most
ofthe SUM and AVG aggregates use numeric_accum or numeric_avg_accum internally, and the wrapper code basically just
doesdatatype conversion, so testing a few cases seems enough there. What I think we *should* test, but don't do
currently,is whether the core machinery performs the expected calls of the forward and reverse transition function. I
wasthinking about creating an aggregate in the regression tests which simply concatenates all the calls into a string,
e.g.you might get "F:1 F:2 F:3 I:1" if we aggregated 1,2,3 and then removed 1. I think that should be possible with an
SQL-languageforward and inverse transfer function, but I haven't tried. I can try, if you want. 

best regards,
Florian Pflug