The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: tested, failed Documentation: tested, failed
This is a review of a patch "Optimizing numeric SUM() aggregate" by Heikki Linnakangas https://www.postgresql.org/message-id/flat/c0545351-a467-5b76-6d46-4840d1ea8...@iki.fi#c0545351-a467-5b76-6d46-4840d1ea8...@iki.fi This review contains summarization of all my posts regarding this patch and a little bit of additional suggestions. Contents & Purpose ================== This patch improves performance of aggregates computation by delaying numeric overflow carring between NBASE-digit arbitrary-length arithmetic. This is possible because 32-bit int is used for storage of every NBASE-digit, where NBASE is 10000. Patch changes file numeric.c only. Folder of numeric.c does not contain README. That is why all documentation of a patch is done in comments. I consider documentation sufficient. Initial Run =========== Patch can be applied cleanly to current HEAD. Regression tests path before and after patch application. Performance =========== I've tested patch with this query CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM generate_series(1, 1000000) s; SELECT avg(d) a , avg(d+0) s0 , avg(d+1) s1 , avg(d+2) s2, avg(d+3) s3 , avg(d+4) s4 , avg(d+5) s5, avg(d+6) s6 , avg(d+7) s7, avg(d+8) s8 , avg(d+9) s9 FROM avg_test; Test specs were: Ubuntu 14 LTS VM, dynamic RAM, hypervisor Windows Server 2016, SSD disk, core i5-2500. Configuration: out of the box master make. On 10 test runs timing of select statement: AVG 3739.8 ms, STD 435.4193 With patch applied (as is) : 3017.8 ms, STD 319.893 I suppose this proves performance impact of a patch. I don't think that sum calculation was a common bottleneck, but certainly patch will slightly improve performance of a very huge number of queries. Suggestions =========== 1. Currenlty overflow is carried every 9999 addition. I suggest that it is possibe to do it only every (INT32_MAX - INT32_MAX / NBASE) / (NBASE - 1) addition. Please note that with this overflow count it will be neccesary to check whether two finishing carrings are required. 2. Aggregates and numeric regression tests do not containt any test case covering overflows. I recommend adding sum with numer 1 000 000 of 99 999 999 values. May be you will come up with more clever overflow testing. Conclusion ========== This patch is important and thorough work, but I'd suggest to consider some polishing on two points listed above. The new status of this patch is: Waiting on Author -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
