Re: Improving avg performance for numeric

Hi,

I've reviewed the v6 of the "numeric optimize" patch
(http://www.postgresql.org/message-id/CAFj8pRDQhG7Pqmf8XqXY0PnHfakkPQLPHnoRLJ_=EKFSbOAWeA@mail.gmail.com)
as Pavel did some hacking on the patch and asked me to do the review.

The patch seems fine to me, the following comments are mostly nitpicking:

1) Applies cleanly to the HEAD (although only by "patch" and not "git
apply").

2) I think we should use "estimate" instead of "approximation" in the
docs, it seems more correct / natural to me (but maybe I'm wrong on this
one).

3) state_data_size does not make much sense to me - it should be
state_size. This probably comes from the state_data_type, but that's
('state' + 'data type') and by replacing the second part with 'size'
you'll get state_size.

4) currently the state size may be specified for all data types, no
matter if their size is fixed or variable. Wouldn't it be safer to allow
this option only for variable-length data types? Right now you can
specify stype=int and sspace=200 which does not make much sense to me.
We can always relax the restrictions if needed, the opposite is much
more difficult.

5) in numeric.c, there are no comments for
- fields of the NumericAggState (some are obvious, some are not)
- multiple functions are missing the initial comments (e.g.
numeric_accum, do_numeric_accum)

6) I think the "first" variable in do_numeric_accum is rather useless,
it's trivial to remove it - just use the state->first instead and move
the assignment to the proper branch (ok, this is nitpicking).

7) I think the error message in makeNumericAggState should be something
like "This must not be called in non-aggregate context!" or something
like that.

8) The records in pg_aggregate.c are using either 0 (for fixed-length)
or 128. This seems slightly excessive to me. What is the reasoning
behind this? Is that because of the two NumericVar fields?

regards
Tomas