Re: SUM(int2)/SUM(int4) do not detect overflow of the int8 accumulator

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Andrei Lepikhov <lepihov(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Nathan Bossart <nathandbossart(at)gmail(dot)com>
Subject: Re: SUM(int2)/SUM(int4) do not detect overflow of the int8 accumulator
Date: 2026-09-01 07:35:43
Message-ID: AC9E5138-2771-48A1-997D-8538A5FD9E01@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Sep 1, 2026, at 01:31, Andrei Lepikhov <lepihov(at)gmail(dot)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/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Richard Guo 2026-09-01 07:43:08 Assert failure in try_nestloop_path()
Previous Message David Geier 2026-09-01 07:21:34 Re: Add pg_stat_vfdcache view for VFD cache statistics