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

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Andrei Lepikhov <lepihov(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(at)paquier(dot)xyz>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, 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-10-20 23:14:45
Message-ID: CAApHDvqZ=t-2soxrDOnZ-mLBhw1Vkw54C3K+k0Gzpu=Sck4LHA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, 15 Oct 2026 at 23:51, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> I was thinking that this might only be worth fixing on master. I'd say
> it's increasingly likely as we make the software faster and computers
> get faster, plus more efficient tableAMs, will result in overflows
> being more likely in the future. That might mean it's inevitable that
> we will need to fix it at some point.
>
> The concern I have with backpatching is around the new
> PG_RETURN_INPUT() macro. That existing in backbranches but not all
> minor versions of the given release could lead to any extension that
> opts to use the macro not compiling on older minor versions that don't
> have the macro defined.

I've split Andrei's v3 into two patches. 0001 is the new macro to
return the input, and 0002 is Andrei's overflow checks. I ended up
renaming the PG_RETURN_INPUT() to PG_RETURN_ARG() as I thought it
better combined the existing names of PG_GETARG_* and PG_RETURN_*.

I agree with what Andres mentioned about changing things so we don't
support calling transition functions directly, as that would allow
getting rid of various AggCheckCallContext checks that don't need to
fetch an actual context. The best way I've thought to do that requires
a new pg_proc column. "procallable", or by using prokind with
something like PROKIND_INTERNAL. I don't really like the idea of doing
this by forcing ourselves into having internal states for these byval
aggregates as a way to block calling due to the INTERNAL parameter.
That means having to palloc memory for the state that we don't
currently have to do for these aggregates. Looking at the history of
int4_sum(), you can see that when we used to have USE_FLOAT8_BYVAL, we
used to palloc memory for that state for non-USE_FLOAT8_BYVAL builds
and pass by value for USE_FLOAT8_BYVAL builds. It's easy to assume we
did the pass-by-value to avoid the palloc.

0001 also gets rid of a stale comment in int8_sum that is left over
from getting rid of USE_FLOAT8_BYVAL.

David

Attachment Content-Type Size
v4-0001-Add-PG_RETURN_ARG-macro-and-use-it-for-sum-int2-a.patch application/octet-stream 4.5 KB
v4-0002-Detect-overflows-in-various-numerical-aggregate-t.patch application/octet-stream 4.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Manu 2026-10-20 23:17:28 Re: Parallel Apply
Previous Message Peter Smith 2026-10-20 22:58:13 Re: Publication of same parent table with ONLY-ness mismatch should error