Re: Improving avg performance for numeric

Hello

2013/9/22 Tomas Vondra <tv(at)fuzzy(dot)cz>

> Hi,
>
> I've reviewed the v6 of the "numeric optimize" patch
> (http://www.postgresql.org/**message-id/**CAFj8pRDQhG7Pqmf8XqXY0PnHfakkP**
> QLPHnoRLJ_=EKFSbOAWeA(at)mail(dot)**gmail(dot)com<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.
>

This name is consistent with previous field state_data_type - I expected so
this mean 'state data' + 'type'. I am not native speaker, so my position is
not strong, but in this moment I am thinking so state_data_size has a
sense. In this case both variant has sense - 'state data' + type or
'state' + 'data type'.

>
> 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.
>
>
good idea

> 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)
>

ok

>
> 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).
>

ok

>
> 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.
>

I used a "a numeric aggregate function called in non-aggregate context" -
it is similar to other related messages

>
> 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?
>

NumericAggState has 96 bytes - but you have to add a space for digits of
included numeric values (inclued in NumericVar) -- so it is others 16 + 16
= 128. I am not able to specify how much digits will be used exactly - 16
bytes is just good enough estimation - it is not used for memory
allocation, it is used for some planner magic.

>
> regards
> Tomas
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/**mailpref/pgsql-hackers<http://www.postgresql.org/mailpref/pgsql-hackers>
>