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