On 23 Září 2013, 18:18, Pavel Stehule wrote: > Hello > > > 2013/9/22 Tomas Vondra <t...@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_=ekfsboa...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
