SUM(int2)/SUM(int4) do not detect overflow of the int8 accumulator
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253622psql -h localhost -U postgresBuilt from patchset v22 (message #22), September 09, 2026 at 12:11 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253622_22 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253622_22 && git checkout t253622_22Patchset v22 (message #22) is on t253622_22
Hi,
Playing around with variants of parallel aggregation on massive inputs,
I found that sum(int4) silently wraps around on overflow. It looks
like a rare case, but looking around I found that almost all the
aggregates have defensive checks, except the int2 and int4 ones.
The overflow itself is not the big problem. The aggregate is
inconsistent with itself: the combine function of sum(int2)/sum(int4)
is int8pl(), which always throws an overflow error. So the same
overflow is either an error or a silently wrong answer, depending on
where in the plan it happens.
This is definitely not a field bug, and the int8 accumulator was a
deliberate choice, not an oversight. A quick overview shows the
following:
1. bec98a31c55 used Numeric accumulators for integer sum/avg
specifically "to avoid overflow at the cost of being a little
slower".
2. 5f7c2bdb537 then replaced that with int8 "for speed reasons", on
the judgement that "INT8 seems large enough to avoid overflow in
practical situations".
3. 4d6ad31 improved the performance of overflow checks on hot paths.
4. 22b0ccd65d2 treated the absence of such a check for the money type
as a bug and back-patched it.
Can it actually be a problem in production? Tables with a row count
around 10^10-10^11 are quite possible (especially partitioned ones,
which are my primary interest), so with average integer values around
10^8, this could show up in some corner cases, though probably not
often.
The attached patch checks with pg_add_s64_overflow() and reports
overflow the way int8pl() does, in int2_sum(), int4_sum(),
int2_avg_accum(), int4_avg_accum() and int4_avg_combine(). The last
three modify the transition value in place, so the patch computes the
new sum first and updates count and sum together. Otherwise an error
partway through would leave the caller with a state whose count and
sum disagree.
Benchmarking with a direct call to the routine (see the examples in
the regression tests) shows an overhead of about 0.07% on my Intel
MacBook, which is close to nothing.
Patch attached -- happy to hear if I'm missing something.
--
regards, Andrei Lepikhov,
pgEdge
On Sep 1, 2026, at 01:31, Andrei Lepikhov <lepihov@gmail.com> wrote:
Hi,
Playing around with variants of parallel aggregation on massive inputs,
I found that sum(int4) silently wraps around on overflow. It looks
like a rare case, but looking around I found that almost all the
aggregates have defensive checks, except the int2 and int4 ones.The overflow itself is not the big problem. The aggregate is
inconsistent with itself: the combine function of sum(int2)/sum(int4)
is int8pl(), which always throws an overflow error. So the same
overflow is either an error or a silently wrong answer, depending on
where in the plan it happens.This is definitely not a field bug, and the int8 accumulator was a
deliberate choice, not an oversight. A quick overview shows the
following:
1. bec98a31c55 used Numeric accumulators for integer sum/avg
specifically "to avoid overflow at the cost of being a little
slower".
2. 5f7c2bdb537 then replaced that with int8 "for speed reasons", on
the judgement that "INT8 seems large enough to avoid overflow in
practical situations".
3. 4d6ad31 improved the performance of overflow checks on hot paths.
4. 22b0ccd65d2 treated the absence of such a check for the money type
as a bug and back-patched it.Can it actually be a problem in production? Tables with a row count
around 10^10-10^11 are quite possible (especially partitioned ones,
which are my primary interest), so with average integer values around
10^8, this could show up in some corner cases, though probably not
often.The attached patch checks with pg_add_s64_overflow() and reports
overflow the way int8pl() does, in int2_sum(), int4_sum(),
int2_avg_accum(), int4_avg_accum() and int4_avg_combine(). The last
three modify the transition value in place, so the patch computes the
new sum first and updates count and sum together. Otherwise an error
partway through would leave the caller with a state whose count and
sum disagree.Benchmarking with a direct call to the routine (see the examples in
the regression tests) shows an overhead of about 0.07% on my Intel
MacBook, which is close to nothing.Patch attached -- happy to hear if I'm missing something.
--
regards, Andrei Lepikhov,
pgEdge
<v0-0001-Detect-overflow-of-the-int8-accumulator-in-some-a.patch>
Shall we also apply pg_sub_s64_overflow() to int2_avg_accum_inv() and int4_avg_accum_inv()? As they do subtraction:
```
transdata->sum -= newval;
```
If newval is negative, the minus operation may also overflow.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 01/09/2026 09:35, Chao Li wrote:
If newval is negative, the minus operation may also overflow.
I don't think that's the case. An inverted function removes values that were
already included in the aggregate, which usually brings the total closer to zero
instead of pushing it toward the boundary. From what I see, this means there's
no risk of integer overflow.
--
regards, Andrei Lepikhov,
pgEdge
On Sep 1, 2026, at 16:22, Andrei Lepikhov <lepihov@gmail.com> wrote:
On 01/09/2026 09:35, Chao Li wrote:
If newval is negative, the minus operation may also overflow.
I don't think that's the case. An inverted function removes values that were
already included in the aggregate, which usually brings the total closer to zero
instead of pushing it toward the boundary. From what I see, this means there's
no risk of integer overflow.--
regards, Andrei Lepikhov,
pgEdge
I agree that this may not happen during normal aggregate execution, but int2_avg_accum_inv() is also directly callable as a SQL function. For example:
```
evantest=# select int2_avg_accum_inv('{1,9223372036854775807}'::int8[], -1::int2);
int2_avg_accum_inv
--------------------------
{0,-9223372036854775808}
(1 row)
```
It’s showing an overflow for a caller-supplied state. I wouldn’t submit a dedicated patch for this case, but I just thought that, since this patch adds overflow checks to the neighboring transition functions, perhaps it would be worth handling this one at the same time. But, anyway, that’s not a strong comment, it’s up to you.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 02/09/2026 07:42, Chao Li wrote:
On Sep 1, 2026, at 16:22, Andrei Lepikhov <lepihov@gmail.com> wrote:
On 01/09/2026 09:35, Chao Li wrote:
If newval is negative, the minus operation may also overflow.
I don't think that's the case. An inverted function removes values that were
already included in the aggregate, which usually brings the total closer to zero
instead of pushing it toward the boundary. From what I see, this means there's
no risk of integer overflow.I agree that this may not happen during normal aggregate execution, but int2_avg_accum_inv() is also directly callable as a SQL function.
In my mind, we should pay for CPU cycles only in practical cases. So, if this
case deserves an overflow check, it should be an assertion that consumes no
resources in production.
--
regards, Andrei Lepikhov,
pgEdge
On Wed, Sep 02, 2026 at 08:01:56AM +0200, Andrei Lepikhov wrote:
In my mind, we should pay for CPU cycles only in practical cases. So, if this
case deserves an overflow check, it should be an assertion that consumes no
resources in production.
All the patterns you are showing imply direct function calls, which
don't really seem worth bothering about. Are any of these overflow
cases reachable using operators with dedicated casts? I would count
as OK even cases where the sum functions are used in a custom
aggregate, say with a SFUNC set to one of the paths you are pointing
at.
--
Michael
On 02/09/2026 09:37, Michael Paquier wrote:
On Wed, Sep 02, 2026 at 08:01:56AM +0200, Andrei Lepikhov wrote:
In my mind, we should pay for CPU cycles only in practical cases. So, if this
case deserves an overflow check, it should be an assertion that consumes no
resources in production.All the patterns you are showing imply direct function calls, which
don't really seem worth bothering about. Are any of these overflow
cases reachable using operators with dedicated casts? I would count
as OK even cases where the sum functions are used in a custom
aggregate, say with a SFUNC set to one of the paths you are pointing
at.
I use direct calls mainly to make regression tests run faster.
The first case arose during benchmarking built-in SUM(int4) with various
parallelising methods [1]https://www.pgedge.com/blog/do-global-hash-tables-strike-back-in-postgresql (bare research topic) at scale. This may not be a big
issue right now, but as databases get larger, it could become one. I think it's
more likely to happen first in the microcurrency space, where the base unit is a
cent instead of a dollar, especially with very large partitioned tables.
[1]: https://www.pgedge.com/blog/do-global-hash-tables-strike-back-in-postgresql
--
regards, Andrei Lepikhov,
pgEdge
On Wed, Sep 02, 2026 at 10:19:29AM +0200, Andrei Lepikhov wrote:
I use direct calls mainly to make regression tests run faster.
The first case arose during benchmarking built-in SUM(int4) with various
parallelising methods [1] (bare research topic) at scale. This may not be a big
issue right now, but as databases get larger, it could become one. I think it's
more likely to happen first in the microcurrency space, where the base unit is a
cent instead of a dollar, especially with very large partitioned tables.[1] https://www.pgedge.com/blog/do-global-hash-tables-strike-back-in-postgresql
Honestly, I don't know how to feel about this patch.
I see the reason why you are doing it for efficiency, but you are
abusing direct function calls (not in the docs) to emulate patterns
that we support behind operators (in user-visible documentation), or
even casts (in user-visible documentation).
Overall I don't see a need to do anything here, but perhaps I'm just
the only one feeling that. Others are free to disagree with this
statement. If there are overflow holes when going through operators
and/or casts, these would be more appealing to fix, IMO, because
these refer to the behavior we expose to the end-users in the docs.
--
Michael
Hi,
On 2026-09-02 13:42:49 +0800, Chao Li wrote:
I agree that this may not happen during normal aggregate execution, but int2_avg_accum_inv() is also directly callable as a SQL function. For example:
```
evantest=# select int2_avg_accum_inv('{1,9223372036854775807}'::int8[], -1::int2);
int2_avg_accum_inv
--------------------------
{0,-9223372036854775808}
(1 row)
```It’s showing an overflow for a caller-supplied state. I wouldn’t submit a
dedicated patch for this case, but I just thought that, since this patch
adds overflow checks to the neighboring transition functions, perhaps it
would be worth handling this one at the same time. But, anyway, that’s not a
strong comment, it’s up to you.
FWIW, I think we should seriously consider making [almost] all the transition
states internal. Having to support calling these functions in non-aggregate
contexts adds complexity without any actual gain. We shouldn't need to check
whether we are in an AggContext, whether the argument is toasted, whether
there are NULL elements in the array, compute offsets into the array, etc.
Greetings,
Andres Freund
On Wed, Sep 02, 2026 at 07:56:32PM -0400, Andres Freund wrote:
FWIW, I think we should seriously consider making [almost] all the transition
states internal. Having to support calling these functions in non-aggregate
contexts adds complexity without any actual gain. We shouldn't need to check
whether we are in an AggContext, whether the argument is toasted, whether
there are NULL elements in the array, compute offsets into the array, etc.
+1.
--
MichaelOn Sep 3, 2026, at 08:10, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Sep 02, 2026 at 07:56:32PM -0400, Andres Freund wrote:
FWIW, I think we should seriously consider making [almost] all the transition
states internal. Having to support calling these functions in non-aggregate
contexts adds complexity without any actual gain. We shouldn't need to check
whether we are in an AggContext, whether the argument is toasted, whether
there are NULL elements in the array, compute offsets into the array, etc.+1. -- Michael
+1
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 03/09/2026 01:44, Michael Paquier wrote:
On Wed, Sep 02, 2026 at 10:19:29AM +0200, Andrei Lepikhov wrote:
I use direct calls mainly to make regression tests run faster.
The first case arose during benchmarking built-in SUM(int4) with various
parallelising methods [1] (bare research topic) at scale. This may not be a big
issue right now, but as databases get larger, it could become one. I think it's
more likely to happen first in the microcurrency space, where the base unit is a
cent instead of a dollar, especially with very large partitioned tables.[1] https://www.pgedge.com/blog/do-global-hash-tables-strike-back-in-postgresql
Honestly, I don't know how to feel about this patch.
I see the reason why you are doing it for efficiency, but you are
abusing direct function calls (not in the docs) to emulate patterns
that we support behind operators (in user-visible documentation), or
even casts (in user-visible documentation).
Ok, no problem. Here is the same thing through the documented path only:
SELECT sum(2147483647::int4) FROM (SELECT generate_series(1, 4294967298)) s;
sum
---------------------
9223372036854775806
(1 row)
Time: 277753.407 ms (04:37.753)
SELECT sum(2147483647::int4) FROM (SELECT generate_series(1, 4294967299)) s;
sum
----------------------
-9223372034707292163
(1 row)
Time: 274398.422 ms (04:34.398)
int8 holds up to 9223372036854775807, so 4294967299 rows of INT_MAX land one row
past the limit. The first query shows the last value that still fits, the second
one silently returns a negative sum.
avg(int4) keeps the same int8 accumulator, and in practical terms it looks worse:
SELECT avg(2147483647::int4) FROM (SELECT generate_series(1, 4294967299)) s;
avg
----------------------
-2147483646.00000000
(1 row)
Time: 341468.965 ms (05:41.469)
The average of 4.3 billion non-negative values comes out negative.
Two details make me think this deserves a fix rather than a documentation note:
* For sum(int4) the answer depends on the plan. int4_sum has no overflow check,
but the combine function is int8pl, which does. So the same query over the same
data returns a wrapped negative number under a serial plan and can fail with
"bigint out of range" under parallel aggregation.
* avg(int4) is not even inconsistent - it is wrong either way. Both
int4_avg_accum and int4_avg_combine add into state->sum unchecked, so no plan
shape turns this into an error. The int2 variants behave the same.
Yes, 4.3 billion rows is a lot to ask of my Intel Macbook laptop - about four
and a half minutes per query on mine. On a modern server reading a large table
it may be reached in reasonable time.
--
regards, Andrei Lepikhov,
pgEdge
On Thu, Sep 03, 2026 at 09:48:31AM +0200, Andrei Lepikhov wrote:
Two details make me think this deserves a fix rather than a documentation note:
* For sum(int4) the answer depends on the plan. int4_sum has no overflow check,
but the combine function is int8pl, which does. So the same query over the same
data returns a wrapped negative number under a serial plan and can fail with
"bigint out of range" under parallel aggregation.* avg(int4) is not even inconsistent - it is wrong either way. Both
int4_avg_accum and int4_avg_combine add into state->sum unchecked, so no plan
shape turns this into an error. The int2 variants behave the same.
It looks to me that you are making your point here, thanks. I can
fall behind that. Even if these functions are marked as internal, we
could still reach the overflows, and I don't find that cool, like you.
A custom aggregate could make the test cheaper and still available
without direct function calls, perhaps, for example with a sfunc =
int{2,4}_sum and an initcond at INT64_MIN/MAX?
Any thoughts or comments from others?
--
Michael
On Fri, 4 Sept 2026 at 16:59, Michael Paquier <michael@paquier.xyz> wrote:
Any thoughts or comments from others?
I think if we don't add overflow error checking for sum(int2) and
sum(int4) today, we'll need to do it at some point in the future. I
suspect we've only gotten away with it for this long, not because
nobody aggregates 4+ billion rows, but because the values being
aggregated are unlikely to be large enough to cause the overflow.
Since Andrei has demonstrated that it's possible to hit that limit
with a non-parallel query in less than 5 minutes, albeit that is
passing INT_MAX (the most extreme case), it might be worth adding the
checks. I was surprised that it only took 5 mins to do 4 billion rows,
especially with generate_series. It's probably just a matter of time
before someone discovers this with a real-world case out in the wild.
If there's a measurable performance regression from adding the
overflow checks, does the attached buy enough of it back? I couldn't
really measure much of a performance difference from it on my Zen2
machine, so I didn't try with the overflow patch.
The patch adds PG_RETURN_INPUT(n) to avoid some of the branching in
int4_sum() so that it immediately returns the aggstate when the value
being aggregated is null. With my compiler, it cut int4_sum from 18
down to 16 instructions.
David
Attachments:
sum_int4_speedup.txttext/plain; charset=US-ASCII; name=sum_int4_speedup.txtDownload+15-12
On 04/09/2026 10:55, David Rowley wrote:
On Fri, 4 Sept 2026 at 16:59, Michael Paquier <michael@paquier.xyz> wrote:
Any thoughts or comments from others?
The patch adds PG_RETURN_INPUT(n) to avoid some of the branching in
int4_sum() so that it immediately returns the aggstate when the value
being aggregated is null. With my compiler, it cut int4_sum from 18
down to 16 instructions.
Thanks for your attention.
I adopted your changes. Although I don't see any overhead beyond noise, it seems
better to optimise than to keep it as is.
Also, tests were rewritten - instead of a direct call, I have used the initcond
trick.
--
regards, Andrei Lepikhov,
pgEdge
Attachments:
t253622_15v1-0001-Detect-overflow-of-the-int8-accumulator-in-sum-an.patchtext/plain; charset=UTF-8; name=v1-0001-Detect-overflow-of-the-int8-accumulator-in-sum-an.patchDownload+149-33
On Fri, Sep 04, 2026 at 08:55:07PM +1200, David Rowley wrote:
Since Andrei has demonstrated that it's possible to hit that limit
with a non-parallel query in less than 5 minutes, albeit that is
passing INT_MAX (the most extreme case), it might be worth adding the
checks. I was surprised that it only took 5 mins to do 4 billion rows,
especially with generate_series. It's probably just a matter of time
before someone discovers this with a real-world case out in the wild.
I guess so..
If there's a measurable performance regression from adding the
overflow checks, does the attached buy enough of it back? I couldn't
really measure much of a performance difference from it on my Zen2
machine, so I didn't try with the overflow patch.The patch adds PG_RETURN_INPUT(n) to avoid some of the branching in
int4_sum() so that it immediately returns the aggstate when the value
being aggregated is null. With my compiler, it cut int4_sum from 18
down to 16 instructions.
FWIW, I've always been a fan of your compiler magic tricks like this
one. Even if you did not measure much of a performance difference at
runtime, less instructions overall across gcc and clang sounds like a
better deal to me anyway? It sounds like the sort of improvements
that could be done independently of what is being discussed here.
--
Michael
On Sat, 5 Sept 2026 at 10:45, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Sep 04, 2026 at 08:55:07PM +1200, David Rowley wrote:
The patch adds PG_RETURN_INPUT(n) to avoid some of the branching in
int4_sum() so that it immediately returns the aggstate when the value
being aggregated is null. With my compiler, it cut int4_sum from 18
down to 16 instructions.FWIW, I've always been a fan of your compiler magic tricks like this
one. Even if you did not measure much of a performance difference at
runtime, less instructions overall across gcc and clang sounds like a
better deal to me anyway? It sounds like the sort of improvements
that could be done independently of what is being discussed here.
Thanks. I thought that the only possible reason for not adding the
overflow checks would be for not wanting to add the performance
penalty of the extra jump for something that's quite unlikely. I
thought we could make that argument void if we found a way to remove
one of the existing jumps so that we kept the same number after
including the overflow checks. If the additional overhead of the
overflow checks is not a concern, then I see no reason not to add
them.
Looking at the latest patch, I'd put this comment back to what I wrote:
+ * Return the running sum unchanged if the new input is null. This also
+ * covers the case where no non-null input has been seen yet, as the
+ * running sum is null then too.The extra sentence has an awful AI whiff to it.
David
David Rowley <dgrowleyml@gmail.com> writes:
Looking at the latest patch, I'd put this comment back to what I wrote:
+ * Return the running sum unchanged if the new input is null. This also + * covers the case where no non-null input has been seen yet, as the + * running sum is null then too.
The extra sentence has an awful AI whiff to it.
The later comments could use more thought too. In particular,
I do not like this sort of pattern:
/* X is true. */
if (x)
// do something
To my mind it's more sensible as
if (x)
{
/* X is true. */
// do something
}
So in these bits:
+ /* This is the first non-null input. */
if (PG_ARGISNULL(0))
- {
- /* No non-null input seen so far... */the replacement comment is badly placed.
At a less nit-picky level:
* In the avg_accum functions, we can argue about how likely it
is that we'd reach overflow of the "sum" fields, but it is completely
insane to expend cycles and code complexity to check for overflow of
the "count" fields. If you can reach 2^63 by repeated addition of 1
within the lifetime of a PG database, then we have got far worse
problems, eg with WAL LSN overflow.
* PG_RETURN_INPUT is not laid out per our usual conventions.
If you need a do/while wrapper, start it on the next line.
* I would not include one single one of these test cases.
They are not worth the development effort nor the forevermore
test runtime cost, especially since they are testing faked-up
scenarios.
regards, tom lane
On Sun, 6 Sept 2026 at 05:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* In the avg_accum functions, we can argue about how likely it
is that we'd reach overflow of the "sum" fields, but it is completely
insane to expend cycles and code complexity to check for overflow of
the "count" fields. If you can reach 2^63 by repeated addition of 1
within the lifetime of a PG database, then we have got far worse
problems, eg with WAL LSN overflow.
I thought the same thing and wondered where that came from. On looking
at int8inc(), I saw there's a similar check, which seems equally
unlikely to hit, so I didn't mention it.
David
On Mon, 7 Sept 2026 at 15:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
On Sun, 6 Sept 2026 at 05:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
* In the avg_accum functions, we can argue about how likely it
is that we'd reach overflow of the "sum" fields, but it is completely
insane to expend cycles and code complexity to check for overflow of
the "count" fields. If you can reach 2^63 by repeated addition of 1
within the lifetime of a PG database, then we have got far worse
problems, eg with WAL LSN overflow.I thought the same thing and wondered where that came from. On looking
at int8inc(), I saw there's a similar check, which seems equally
unlikely to hit, so I didn't mention it.int8inc doesn't really have a basis to suppose that it's starting
from count zero, does it? It's a SQL-accessible function defined
as "add one".
That's a good point. I now agree that checking for overflow on the
count for the avg_accum functions is a waste of effort.
David
Import Notes
Reply to msg id not found: 156738.1788751287@sss.pgh.pa.us