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
From 76d9f5ad76494ce02febfdecef0aa83a8fd8667a Mon Sep 17 00:00:00 2001 From: "Andrei V. Lepikhov" <[email protected]> Date: Mon, 31 Aug 2026 15:50:41 +0200 Subject: [PATCH v0] Detect overflow of the int8 accumulator in some aggregate functions The sum(int2) and sum(int4) functions use an int8 value to store their results, since int8 should be large enough for most cases. Until now, the code just added values together without checking for overflow. If overflow occurred, the result would silently wrap around, leading to an incorrect answer. This made the aggregate behave inconsistently. The combine function, int8pl(), has always given a "bigint out of range" error when there is an overflow. The moving-aggregate path uses intX_avg_accum() with avg(), so the same overflow could either cause an error or just wrap around, depending on whether the planner chose a parallel plan. Signed overflow is also undefined behavior, so we cannot trust the wrapped value. One of the main goals of 101c7ee3ee8 was to remove these cases. Now, we check for overflow using pg_add_s64_overflow() and report it in the same way that int8pl() does. The intX_avg_accum() and int4_avg_combine() functions calculate the new sum first, and then update both the count and sum together. This prevents errors that could leave the count and sum out of sync. This change also adds the overflow check to avg(int2) and avg(int4), since they use the same transition functions. --- src/backend/utils/adt/numeric.c | 39 +++++++++++++++++++++--- src/test/regress/expected/aggregates.out | 19 ++++++++++++ src/test/regress/sql/aggregates.sql | 12 ++++++++ 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index 523cd3cd608..a48dab3779c 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -6367,7 +6367,11 @@ int2_sum(PG_FUNCTION_ARGS) PG_RETURN_INT64(oldsum); /* OK to do the addition. */ - newval = oldsum + (int64) PG_GETARG_INT16(1); + if (unlikely(pg_add_s64_overflow(oldsum, (int64) PG_GETARG_INT16(1), + &newval))) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("bigint out of range"))); PG_RETURN_INT64(newval); } @@ -6395,7 +6399,11 @@ int4_sum(PG_FUNCTION_ARGS) PG_RETURN_INT64(oldsum); /* OK to do the addition. */ - newval = oldsum + (int64) PG_GETARG_INT32(1); + if (unlikely(pg_add_s64_overflow(oldsum, (int64) PG_GETARG_INT32(1), + &newval))) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("bigint out of range"))); PG_RETURN_INT64(newval); } @@ -6457,6 +6465,7 @@ int2_avg_accum(PG_FUNCTION_ARGS) ArrayType *transarray; int16 newval = PG_GETARG_INT16(1); Int8TransTypeData *transdata; + int64 newsum; /* * If we're invoked as an aggregate, we can cheat and modify our first @@ -6473,8 +6482,14 @@ int2_avg_accum(PG_FUNCTION_ARGS) elog(ERROR, "expected 2-element int8 array"); transdata = (Int8TransTypeData *) ARR_DATA_PTR(transarray); + + if (unlikely(pg_add_s64_overflow(transdata->sum, (int64) newval, &newsum))) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("bigint out of range"))); + transdata->count++; - transdata->sum += newval; + transdata->sum = newsum; PG_RETURN_ARRAYTYPE_P(transarray); } @@ -6485,6 +6500,7 @@ int4_avg_accum(PG_FUNCTION_ARGS) ArrayType *transarray; int32 newval = PG_GETARG_INT32(1); Int8TransTypeData *transdata; + int64 newsum; /* * If we're invoked as an aggregate, we can cheat and modify our first @@ -6501,8 +6517,14 @@ int4_avg_accum(PG_FUNCTION_ARGS) elog(ERROR, "expected 2-element int8 array"); transdata = (Int8TransTypeData *) ARR_DATA_PTR(transarray); + + if (unlikely(pg_add_s64_overflow(transdata->sum, (int64) newval, &newsum))) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("bigint out of range"))); + transdata->count++; - transdata->sum += newval; + transdata->sum = newsum; PG_RETURN_ARRAYTYPE_P(transarray); } @@ -6514,6 +6536,7 @@ int4_avg_combine(PG_FUNCTION_ARGS) ArrayType *transarray2; Int8TransTypeData *state1; Int8TransTypeData *state2; + int64 newsum; if (!AggCheckCallContext(fcinfo, NULL)) elog(ERROR, "aggregate function called in non-aggregate context"); @@ -6532,8 +6555,14 @@ int4_avg_combine(PG_FUNCTION_ARGS) state1 = (Int8TransTypeData *) ARR_DATA_PTR(transarray1); state2 = (Int8TransTypeData *) ARR_DATA_PTR(transarray2); + /* As in intX_avg_accum, don't modify state1 until we know we can. */ + if (unlikely(pg_add_s64_overflow(state1->sum, state2->sum, &newsum))) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("bigint out of range"))); + state1->count += state2->count; - state1->sum += state2->sum; + state1->sum = newsum; PG_RETURN_ARRAYTYPE_P(transarray1); } diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 7d07619956f..d33d2f92a15 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -87,6 +87,25 @@ SELECT sum(gpa) AS avg_6_8 FROM ONLY student; 6.8 (1 row) +-- sum(int2) and sum(int4) accumulate into int8. Overflowing that accumulator +-- would take billions of rows, so exercise the transition functions directly. +SELECT int2_sum('9223372036854775807'::int8, 1::int2); +ERROR: bigint out of range +SELECT int4_sum('9223372036854775807'::int8, 1::int4); +ERROR: bigint out of range +SELECT int2_avg_accum('{0,9223372036854775807}'::int8[], 1::int2); +ERROR: bigint out of range +SELECT int4_avg_accum('{0,9223372036854775807}'::int8[], 1::int4); +ERROR: bigint out of range +-- negative inputs must be caught going the other way just the same +SELECT int2_sum('-9223372036854775808'::int8, -1::int2); +ERROR: bigint out of range +SELECT int4_sum('-9223372036854775808'::int8, -1::int4); +ERROR: bigint out of range +SELECT int2_avg_accum('{0,-9223372036854775808}'::int8[], -1::int2); +ERROR: bigint out of range +SELECT int4_avg_accum('{0,-9223372036854775808}'::int8[], -1::int4); +ERROR: bigint out of range SELECT max(four) AS max_3 FROM onek; max_3 ------- diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 91f8342166f..264dedd8496 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -42,6 +42,18 @@ SELECT sum(a) AS sum_198 FROM aggtest; SELECT sum(b) AS avg_431_773 FROM aggtest; SELECT sum(gpa) AS avg_6_8 FROM ONLY student; +-- sum(int2) and sum(int4) accumulate into int8. Overflowing that accumulator +-- would take billions of rows, so exercise the transition functions directly. +SELECT int2_sum('9223372036854775807'::int8, 1::int2); +SELECT int4_sum('9223372036854775807'::int8, 1::int4); +SELECT int2_avg_accum('{0,9223372036854775807}'::int8[], 1::int2); +SELECT int4_avg_accum('{0,9223372036854775807}'::int8[], 1::int4); +-- negative inputs must be caught going the other way just the same +SELECT int2_sum('-9223372036854775808'::int8, -1::int2); +SELECT int4_sum('-9223372036854775808'::int8, -1::int4); +SELECT int2_avg_accum('{0,-9223372036854775808}'::int8[], -1::int2); +SELECT int4_avg_accum('{0,-9223372036854775808}'::int8[], -1::int4); + SELECT max(four) AS max_3 FROM onek; SELECT max(a) AS max_100 FROM aggtest; SELECT max(aggtest.b) AS max_324_78 FROM aggtest; -- 2.55.0
