Thanks for the review! On Mon, Aug 31, 2026 at 3:26 PM Michael Paquier <[email protected]> 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
v2-0001-Use-pg_neg_s-16-32-64-_overflow-for-open-coded-negat.patch
Description: Binary data
v2-0002-Drop-redundant-unlikely-around-overflow-checks-that-.patch
Description: Binary data
