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

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.

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.

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 (
To make changes to your subscription:

Reply via email to