On 23 Září 2013, 18:18, Pavel Stehule wrote:
> Hello
>
>
> 2013/9/22 Tomas Vondra <tv@fuzzy.cz>
>
>> Hi,
>>
>> I've reviewed the v6 of the "numeric optimize" patch
>> (http://www.postgresql.org/**message-id/**CAFj8pRDQhG7Pqmf8XqXY0PnHfakkP**
>>
QLPHnoRLJ_=EKFSbOAWeA@mail.**gmail.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'.
OK, let's leave this up to a native speaker.
>>
>> 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.
OK, makes sense.
I've made some basic tests on a 40M table with random numerics, for
example this query:
select avg(val), sum(val), var_pop(val) from numeric_test ;
takes ~57 seconds on current master, but only ~26 seconds with the v7
patch. Granted, in practice the improvements won't be as good because of
I/O costs etc., but it's a nice gain.
Seems "ready for commiter" to me. I'll wait a few days for others to
comment, and then I'll update the commitfest page.
regards
Tomas