Re: Use pg_neg_s*_overflow() for open-coded negation overflow checks

Thanks for the review!

On Mon, Aug 31, 2026 at 3:26 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, Aug 27, 2026 at 01:57:59PM +0800, Ewan Young wrote:
> > No behavioral change: each site keeps its existing hard- or soft-error
> > path.
>
>
> - if (value == PG_INT64_MIN)
> + if (pg_neg_s64_overflow(value, &result))
>
> One could ask why you don't add an unlikely() here. But it looks to
> me that this locaton is right and that you could just remove the
> unlikely() from all the places where the overflow routines are used
> anyway, because they already embed some unlikely() calls for the
> minimum bound checks.

Agreed, with one refinement. The embedded unlikely() only exists in
the helpers' non-builtin fallback; on the __builtin_*_overflow path
there is no hint inside the helper. What actually makes the hint
redundant at most of these sites is that the failure branch ends in
ereport(ERROR): a constant elevel >= ERROR goes through
errstart_cold(), so the compiler already treats the branch as cold.

So in v2 this is a two-patch series:

0001 is the conversion. Converted sites whose branch raises
ereport(ERROR) use the helper bare. numericvar_to_int64() keeps its
unlikely(), because its failure branch is a plain "return false" with
no cold marking -- and the adjacent pg_mul/pg_sub_s64_overflow() calls
a few lines up in the same function keep theirs, so dropping only the
new one would trade one inconsistency for another.

0002 then removes the now-redundant unlikely() from the other
pg_{add,sub,mul}_s*_overflow() call sites in the touched files whose
branch is ereport(ERROR) -- which is, I think, the sweep your comment
was inviting. It deliberately keeps the hint where the failure branch
is ordinary code with no cold marking: the in_range() handling in
int.c/int8.c, the 128-bit fallback and numericvar_to_int64() in
numeric.c, and int4_cash()/int8_cash() (ereturn, i.e. errsave, is not
cold-marked, since the soft path can be taken routinely). I also left
the pg_neg_u* callers in numutils.c alone for the same reason (soft
goto in hot parsing code). If you'd rather see the sweep tree-wide,
or not at all, 0002 is easy to adjust or drop.

>
> + if (arg1 < 0 && unlikely(pg_neg_s32_overflow(arg1, &result)))
> ereport(ERROR,
>
> Well, this should embed both conditions but we don't need an unlikely
> here anyway. That could also be written as the following, which seems
> slightly better from here in terms of the abs functions:
> if (arg1 < 0)
> {
> if (neg_overflow())
> ereport(ERROR)
> }
> else
> result = arg1;
>

Done that way in v2; it reads better than the result-preassignment
trick, agreed.

> - /* If the result is INT64_MIN, it cannot be represented. */
>
> Comment removed. Perhaps it should not.

Restored, in both int4lcm() and int8lcm().

>
> - if (unlikely(c == PG_INT64_MIN))
> + Cash result;
>
> This pattern introduced in cash_div_int64() is inconsistent with the
> code in cash_mul_int64() a couple of lines above.

Fixed: cash_div_int64() now declares "Cash res" at the top and uses
the same shape as cash_mul_int64() (whose unlikely() is then removed
by 0002 along with its siblings, keeping the file uniform).

Tested: make check with and without HAVE__BUILTIN_OP_OVERFLOW, plus
manual runs of every converted function at the INT16/32/64_MIN
boundaries (unary minus, abs, division by -1, lcm, money, cash_in,
numeric->int8, and interval '... ago' with INT_MIN month/usec fields):
behavior is unchanged everywhere.

> --
> Michael

--
Regards,
Ewan Young