Subject: Re: Remove dependence on integer wrapping

From: jian he <[email-masked]>
Date: Fri, 9 Aug 2024 09:01:27 +0800

On Fri, Aug 9, 2024 at 6:16 AM Matthew Kim <[email-masked]> wrote:
>
> I've updated patch 0004 to check the return value of pg_mul_s32_overflow(). Since tm.tm_year overflowed, the error message is hardcoded.
>

--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -257,7 +257,10 @@ make_date(PG_FUNCTION_ARGS)
  if (tm.tm_year < 0)
  {
  bc = true;
- tm.tm_year = -tm.tm_year;
+ if (pg_mul_s32_overflow(tm.tm_year, -1, &tm.tm_year))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("date field value out of range")));
  }

Should the error about integers be out of range?

SELECT make_date(-2147483648, 1, 1);
"-2147483648" is not an allowed integer.

\df make_date
                                      List of functions
   Schema   |   Name    | Result data type |           Argument data
types            | Type
------------+-----------+------------------+------------------------------------------+------
 pg_catalog | make_date | date             | year integer, month
integer, day integer | func

---

From: Joseph Koshakow <[email-masked]>
Date: Sat, 10 Aug 2024 11:41:31 -0400

> integer, day integer | func

Are you saying that with the patch applied you're seeing the above
error? If so, I see a different error.

    test=# SELECT make_date(-2147483648, 1, 1);
    ERROR:  date field value out of range

Or are you saying that we should change the code in the patch so that
it returns the above error? If so, I'm not sure I understand the
reasoning. -2147483648 is an allowed integer, it's the minimum allowed
value for integers.

    test=# SELECT (-2147483648)::integer;
        int4
    -------------
     -2147483648
    (1 row)

Thanks,
Joseph Koshakow

---

From: Matthew Kim <[email-masked]>
Date: Sat, 10 Aug 2024 14:54:48 -0700

On Mon, Jul 22, 2024 at 5:52 PM Alexander Lakhin
<exclusion(at)gmail(dot)com>
wrote:

> Also there are several trap-producing cases with date types:
> SELECT to_timestamp('1000000000,999', 'Y,YYY');

I attached patch 5 that fixes the to_timestamp overflow.

Thank you,
Matthew Kim

---

From: jian he <[email-masked]>
Date: Mon, 12 Aug 2024 18:33:13 +0800

>     test=# SELECT (-2147483648)::integer;

sorry, i mixed up
select (-2147483648)::int;
with
select -2147483648::int;

looks good to me.
maybe make it more explicit:
errmsg("date field (year) value out of range")));
i don't have a huge opinion though.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 13 Aug 2024 16:46:34 -0500

I've been preparing 0001 for commit.  I've attached what I have so far.

The main changes are the implementations of pg_abs_* and pg_neg_*.  For the
former, I've used abs()/i64abs() for the short/int implementations.  For
the latter, I've tried to use __builtin_sub_overflow() when possible, as
that appears to produce slightly better code.  When
__builtin_sub_overflow() is not available, the values are upcasted before
negation, and we check that result before casting to the return type.  That
approach more closely matches the surrounding functions.  (One exception is
pg_neg_u64_overflow() when we have neither HAVE__BUILTIN_OP_OVERFLOW nor
HAVE_INT128.  In that case, we have to hand-roll everything.)

Thoughts?

-- 
nathan
>From c4e67e1f32562037dbc83baca5bcda00e0fcd0d7 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v19 1/1] Remove dependence on integer wrapping

This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
 src/backend/utils/adt/cash.c               | 12 +--
 src/backend/utils/adt/numeric.c            |  4 +-
 src/backend/utils/adt/numutils.c           | 34 ++++----
 src/backend/utils/adt/timestamp.c          | 28 +------
 src/include/common/int.h                   | 92 ++++++++++++++++++++++
 src/interfaces/ecpg/pgtypeslib/timestamp.c | 11 +--
 src/test/regress/expected/timestamp.out    | 13 +++
 src/test/regress/expected/timestamptz.out  | 13 +++
 src/test/regress/sql/timestamp.sql         |  4 +
 src/test/regress/sql/timestamptz.sql       |  4 +
 10 files changed, 158 insertions(+), 57 deletions(-)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index ec3c08acfc..c1a743b2a6 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -387,6 +387,7 @@ Datum
 cash_out(PG_FUNCTION_ARGS)
 {
 	Cash		value = PG_GETARG_CASH(0);
+	uint64		uvalue;
 	char	   *result;
 	char		buf[128];
 	char	   *bufptr;
@@ -429,8 +430,6 @@ cash_out(PG_FUNCTION_ARGS)
 
 	if (value < 0)
 	{
-		/* make the amount positive for digit-reconstruction loop */
-		value = -value;
 		/* set up formatting data */
 		signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
 		sign_posn = lconvert->n_sign_posn;
@@ -445,6 +444,9 @@ cash_out(PG_FUNCTION_ARGS)
 		sep_by_space = lconvert->p_sep_by_space;
 	}
 
+	/* make the amount positive for digit-reconstruction loop */
+	uvalue = pg_abs_s64(value);
+
 	/* we build the digits+decimal-point+sep string right-to-left in buf[] */
 	bufptr = buf + sizeof(buf) - 1;
 	*bufptr = '\0';
@@ -470,10 +472,10 @@ cash_out(PG_FUNCTION_ARGS)
 			memcpy(bufptr, ssymbol, strlen(ssymbol));
 		}
 
-		*(--bufptr) = ((uint64) value % 10) + '0';
-		value = ((uint64) value) / 10;
+		*(--bufptr) = (uvalue % 10) + '0';
+		uvalue = uvalue / 10;
 		digit_pos--;
-	} while (value || digit_pos >= 0);
+	} while (uvalue || digit_pos >= 0);
 
 	/*----------
 	 * Now, attach currency symbol and sign symbol in the correct order.
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index d0f0923710..3d69e77998 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8117,7 +8117,7 @@ int64_to_numericvar(int64 val, NumericVar *var)
 	if (val < 0)
 	{
 		var->sign = NUMERIC_NEG;
-		uval = -val;
+		uval = pg_abs_s64(val);
 	}
 	else
 	{
@@ -11443,7 +11443,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
 	 * Now we can proceed with the multiplications.
 	 */
 	neg = (exp < 0);
-	mask = abs(exp);
+	mask = pg_abs_s32(exp);
 
 	init_var(&base_prod);
 	set_var_from_var(base, &base_prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
 #include <limits.h>
 #include <ctype.h>
 
+#include "common/int.h"
 #include "port/pg_bitutils.h"
 #include "utils/builtins.h"
 
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
 	uint16		tmp = 0;
 	bool		neg = false;
 	unsigned char digit;
+	int16		result;
 
 	/*
 	 * The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+		if (pg_neg_u16_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int16) tmp);
+		return result;
 	}
 
 	if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+		if (pg_neg_u16_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int16) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT16_MAX)
@@ -393,6 +393,7 @@ pg_strtoint32_safe(const char *s, Node *escontext)
 	uint32		tmp = 0;
 	bool		neg = false;
 	unsigned char digit;
+	int32		result;
 
 	/*
 	 * The majority of cases are likely to be base-10 digits without any
@@ -452,10 +453,9 @@ pg_strtoint32_safe(const char *s, Node *escontext)
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1))
+		if (pg_neg_u32_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int32) tmp);
+		return result;
 	}
 
 	if (unlikely(tmp > PG_INT32_MAX))
@@ -595,10 +595,9 @@ slow:
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
+		if (pg_neg_u32_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int32) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT32_MAX)
@@ -655,6 +654,7 @@ pg_strtoint64_safe(const char *s, Node *escontext)
 	uint64		tmp = 0;
 	bool		neg = false;
 	unsigned char digit;
+	int64		result;
 
 	/*
 	 * The majority of cases are likely to be base-10 digits without any
@@ -714,10 +714,9 @@ pg_strtoint64_safe(const char *s, Node *escontext)
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1))
+		if (pg_neg_u64_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int64) tmp);
+		return result;
 	}
 
 	if (unlikely(tmp > PG_INT64_MAX))
@@ -857,10 +856,9 @@ slow:
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1)
+		if (pg_neg_u64_overflow(tmp, &result))
 			goto out_of_range;
-		return -((int64) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT64_MAX)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 69fe7860ed..c76793f72d 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -618,19 +618,8 @@ make_timestamp_internal(int year, int month, int day,
 	time = (((hour * MINS_PER_HOUR + min) * SECS_PER_MINUTE)
 			* USECS_PER_SEC) + (int64) rint(sec * USECS_PER_SEC);
 
-	result = date * USECS_PER_DAY + time;
-	/* check for major overflow */
-	if ((result - time) / USECS_PER_DAY != date)
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g",
-						year, month, day,
-						hour, min, sec)));
-
-	/* check for just-barely overflow (okay except time-of-day wraps) */
-	/* caution: we want to allow 1999-12-31 24:00:00 */
-	if ((result < 0 && date > 0) ||
-		(result > 0 && date < -1))
+	if (pg_mul_s64_overflow(date, USECS_PER_DAY, &result) ||
+		pg_add_s64_overflow(result, time, &result))
 		ereport(ERROR,
 				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 				 errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g",
@@ -2010,17 +1999,8 @@ tm2timestamp(struct pg_tm *tm, fsec_t fsec, int *tzp, Timestamp *result)
 	date = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - POSTGRES_EPOCH_JDATE;
 	time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec);
 
-	*result = date * USECS_PER_DAY + time;
-	/* check for major overflow */
-	if ((*result - time) / USECS_PER_DAY != date)
-	{
-		*result = 0;			/* keep compiler quiet */
-		return -1;
-	}
-	/* check for just-barely overflow (okay except time-of-day wraps) */
-	/* caution: we want to allow 1999-12-31 24:00:00 */
-	if ((*result < 0 && date > 0) ||
-		(*result > 0 && date < -1))
+	if (pg_mul_s64_overflow(date, USECS_PER_DAY, result) ||
+		pg_add_s64_overflow(*result, time, result))
 	{
 		*result = 0;			/* keep compiler quiet */
 		return -1;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..961e52e9ad 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -32,6 +32,11 @@
  * - If a * b overflows, return true, otherwise store the result of a * b
  * into *result. The content of *result is implementation defined in case of
  * overflow.
+ * - If -a overflows, return true, otherwise store the result of -a into
+ * *result. The content of *result is implementation defined in case of
+ * overflow.
+ * - Return the absolute value of a as an unsigned integer of the same
+ * width.
  *---------
  */
 
@@ -97,6 +102,12 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline uint16
+pg_abs_s16(int16 a)
+{
+	return abs(a);
+}
+
 /*
  * INT32
  */
@@ -154,6 +165,12 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
 #endif
 }
 
+static inline uint32
+pg_abs_s32(int32 a)
+{
+	return i64abs(a);
+}
+
 /*
  * INT64
  */
@@ -258,6 +275,16 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
 #endif
 }
 
+static inline uint64
+pg_abs_s64(int64 a)
+{
+	if (unlikely(a == PG_INT64_MIN))
+		return (uint64) PG_INT64_MAX + 1;
+	if (a < 0)
+		return -a;
+	return a;
+}
+
 /*------------------------------------------------------------------------
  * Overflow routines for unsigned integers
  *------------------------------------------------------------------------
@@ -318,6 +345,24 @@ pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result)
 #endif
 }
 
+static inline bool
+pg_neg_u16_overflow(uint16 a, int16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	int32		res = -((int32) a);
+
+	if (unlikely(a > PG_INT16_MAX))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = (int16) res;
+	return false;
+#endif
+}
+
 /*
  * INT32
  */
@@ -373,6 +418,24 @@ pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result)
 #endif
 }
 
+static inline bool
+pg_neg_u32_overflow(uint32 a, int32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	int64		res = -((int64) a);
+
+	if (unlikely(res > PG_INT32_MAX))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = (int32) res;
+	return false;
+#endif
+}
+
 /*
  * UINT64
  */
@@ -438,6 +501,35 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+static inline bool
+pg_neg_u64_overflow(uint64 a, int64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#elif defined(HAVE_INT128)
+	uint128		res = -((int128) a);
+
+	if (unlikely(res > PG_INT64_MAX))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = (int64) res;
+	return false;
+#else
+	if (unlikely(a > (uint64) PG_INT64_MAX + 1))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	if (unlikely(a == (uint64) PG_INT64_MAX + 1))
+		*result = PG_INT64_MIN;
+	else
+		*result = -((int64) a);
+	return false;
+#endif
+}
+
 /*------------------------------------------------------------------------
  *
  * Comparison routines for integer types.
diff --git a/src/interfaces/ecpg/pgtypeslib/timestamp.c b/src/interfaces/ecpg/pgtypeslib/timestamp.c
index f1b143fbd2..93d4cc323d 100644
--- a/src/interfaces/ecpg/pgtypeslib/timestamp.c
+++ b/src/interfaces/ecpg/pgtypeslib/timestamp.c
@@ -11,6 +11,7 @@
 #error -ffast-math is known to break this code
 #endif
 
+#include "common/int.h"
 #include "dt.h"
 #include "pgtypes_date.h"
 #include "pgtypes_timestamp.h"
@@ -48,14 +49,8 @@ tm2timestamp(struct tm *tm, fsec_t fsec, int *tzp, timestamp * result)
 
 	dDate = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - date2j(2000, 1, 1);
 	time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec);
-	*result = (dDate * USECS_PER_DAY) + time;
-	/* check for major overflow */
-	if ((*result - time) / USECS_PER_DAY != dDate)
-		return -1;
-	/* check for just-barely overflow (okay except time-of-day wraps) */
-	/* caution: we want to allow 1999-12-31 24:00:00 */
-	if ((*result < 0 && dDate > 0) ||
-		(*result > 0 && dDate < -1))
+	if (pg_mul_s64_overflow(dDate, USECS_PER_DAY, result) ||
+		pg_add_s64_overflow(*result, time, result))
 		return -1;
 	if (tzp != NULL)
 		*result = dt2local(*result, -(*tzp));
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out
index cf337da517..e287260051 100644
--- a/src/test/regress/expected/timestamp.out
+++ b/src/test/regress/expected/timestamp.out
@@ -2201,3 +2201,16 @@ select age(timestamp '-infinity', timestamp 'infinity');
 
 select age(timestamp '-infinity', timestamp '-infinity');
 ERROR:  interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+        timestamp         
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
+select make_timestamp(1999, 12, 31, 24, 0, 0);
+      make_timestamp      
+--------------------------
+ Sat Jan 01 00:00:00 2000
+(1 row)
+
diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out
index bfb3825ff6..d01d174983 100644
--- a/src/test/regress/expected/timestamptz.out
+++ b/src/test/regress/expected/timestamptz.out
@@ -3286,3 +3286,16 @@ SELECT age(timestamptz '-infinity', timestamptz 'infinity');
 
 SELECT age(timestamptz '-infinity', timestamptz '-infinity');
 ERROR:  interval out of range
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+         timestamptz          
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
+       make_timestamptz       
+------------------------------
+ Sat Jan 01 00:00:00 2000 PST
+(1 row)
+
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index 820ef7752a..748469576d 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -424,3 +424,7 @@ select age(timestamp 'infinity', timestamp 'infinity');
 select age(timestamp 'infinity', timestamp '-infinity');
 select age(timestamp '-infinity', timestamp 'infinity');
 select age(timestamp '-infinity', timestamp '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamp '1999-12-31 24:00:00';
+select make_timestamp(1999, 12, 31, 24, 0, 0);
diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql
index ccfd90d646..c71d5489b4 100644
--- a/src/test/regress/sql/timestamptz.sql
+++ b/src/test/regress/sql/timestamptz.sql
@@ -668,3 +668,7 @@ SELECT age(timestamptz 'infinity', timestamptz 'infinity');
 SELECT age(timestamptz 'infinity', timestamptz '-infinity');
 SELECT age(timestamptz '-infinity', timestamptz 'infinity');
 SELECT age(timestamptz '-infinity', timestamptz '-infinity');
+
+-- test timestamp near POSTGRES_EPOCH_JDATE
+select timestamptz '1999-12-31 24:00:00';
+select make_timestamptz(1999, 12, 31, 24, 0, 0);
-- 
2.39.3 (Apple Git-146)

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 13 Aug 2024 22:07:44 -0500

> HAVE_INT128.  In that case, we have to hand-roll everything.)

And here's a new version of the patch in which I've attempted to fix the
silly mistakes.

-- 
nathan
>From 2b7b38f764f16b49594d0f8cc6192b7ba74ef906 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v20 1/1] Remove dependence on integer wrapping

 
 	if (value < 0)
 	{

 
 	/*----------
 
 	init_var(&base_prod);
 
+#include "common/int.h"

 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT16_MAX)
 
 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT32_MAX)
 
 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT64_MAX)

index 7fc046e78a..7a4dec4635 100644

+static inline uint16
+pg_abs_s16(int16 a)
+{
+	return abs(a);
+}
+
 /*
  * INT32
  */
 
+static inline uint32
+pg_abs_s32(int32 a)
+{
+	return i64abs(a);
+}
+
 /*
  * INT64
  */
 
+static inline uint64
+pg_abs_s64(int64 a)
+{
 
+static inline bool
+	if (unlikely(res < PG_INT16_MIN))
+	*result = res;
 
+static inline bool
+	if (unlikely(res < PG_INT32_MIN))
+	*result = res;
 
+static inline bool
+	if (unlikely(res < PG_INT64_MIN))
+	*result = res;
 
+#include "common/int.h"
 #include "dt.h"

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 14 Aug 2024 10:02:28 +0300

> + * - Return the absolute value of a as an unsigned integer of the same

The last "Return the absolute value of a ..." sentence feels a bit 
weird. In all the preceding sentences, 'a' is part of an "If a" sentence 
that defines what 'a' is. In the last one, it's kind of just hanging there.

> +	return abs(a);

This is correct, but it took me a while to understand why. Maybe some 
comments would be in order.

The function it calls is "int abs(int)". So this first widens the int16 
to int32, and then narrows the result from int32 to uint16.

The man page for abs() says "Trying to take the absolute value of the 
most negative integer is not defined." That's OK in this case, because 
that refers to the most negative int32 value, and the argument here is 
int16. But that's why the pg_abs_s64(int64) function needs the special 
check for the most negative value.

There's also some code in libpq's pqCheckOutBufferSpace() function that 
could use these functions.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 14 Aug 2024 12:20:40 -0500

> LGTM, just a few small comments:

Thanks for reviewing.

> The last "Return the absolute value of a ..." sentence feels a bit weird. In
> all the preceding sentences, 'a' is part of an "If a" sentence that defines
> what 'a' is. In the last one, it's kind of just hanging there.

How about:

	If a is negative, return -a, otherwise return a.  Overflow cannot occur
	because the return value is an unsigned integer with the same width as
	the argument.

> The function it calls is "int abs(int)". So this first widens the int16 to
> int32, and then narrows the result from int32 to uint16.
> 
> The man page for abs() says "Trying to take the absolute value of the most
> negative integer is not defined." That's OK in this case, because that
> refers to the most negative int32 value, and the argument here is int16. But
> that's why the pg_abs_s64(int64) function needs the special check for the
> most negative value.

Yeah, I've added some casts/comments to make this clear.  I got too excited
about trimming it down and ended up obfuscating these important details.

> could use these functions.

Duly noted.

-- 
nathan
>From 08b32a02629a7628353643960b9956d01a795f18 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v21 1/1] Remove dependence on integer wrapping

This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
 src/backend/utils/adt/cash.c               |  12 ++-
 src/backend/utils/adt/numeric.c            |   4 +-
 src/backend/utils/adt/numutils.c           |  34 ++++---
 src/backend/utils/adt/timestamp.c          |  28 +-----
 src/include/common/int.h                   | 103 +++++++++++++++++++++
 src/interfaces/ecpg/pgtypeslib/timestamp.c |  11 +--
 src/test/regress/expected/timestamp.out    |  13 +++
 src/test/regress/expected/timestamptz.out  |  13 +++
 src/test/regress/sql/timestamp.sql         |   4 +
 src/test/regress/sql/timestamptz.sql       |   4 +
 10 files changed, 169 insertions(+), 57 deletions(-)

 	if (value < 0)
 	{

 
 	/*----------
 	 * Now, attach currency symbol and sign symbol in the correct order.
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 763a7f4be0..5ee001e382 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8111,7 +8111,7 @@ int64_to_numericvar(int64 val, NumericVar *var)
 	if (val < 0)
 	{
 		var->sign = NUMERIC_NEG;
-		uval = -val;
+		uval = pg_abs_s64(val);
 	}
 	else
 	{
@@ -11437,7 +11437,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
 	 * Now we can proceed with the multiplications.
 	 */
 	neg = (exp < 0);
-	mask = abs(exp);
+	mask = pg_abs_s32(exp);
 
 	init_var(&base_prod);
index adc1e8a4cb..63c2beb6a2 100644
 
+#include "common/int.h"

 	/*
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+		if (unlikely(pg_neg_u16_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int16) tmp);
+		return result;
 	}

 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+		if (unlikely(pg_neg_u16_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int16) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT16_MAX)
 
 	/*
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1))
+		if (unlikely(pg_neg_u32_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int32) tmp);
+		return result;
 	}

 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
+		if (unlikely(pg_neg_u32_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int32) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT32_MAX)
 
 	/*
 
 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (unlikely(tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1))
+		if (unlikely(pg_neg_u64_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int64) tmp);
+		return result;
 	}

 	if (neg)
 	{
-		/* check the negative equivalent will fit without overflowing */
-		if (tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1)
+		if (unlikely(pg_neg_u64_overflow(tmp, &result)))
 			goto out_of_range;
-		return -((int64) tmp);
+		return result;
 	}
 
 	if (tmp > PG_INT64_MAX)
index 69fe7860ed..43800addf4 100644
 
+	if (unlikely(pg_mul_s64_overflow(date, USECS_PER_DAY, &result) ||
+				 pg_add_s64_overflow(result, time, &result)))
 
-	*result = date * USECS_PER_DAY + time;
-	/* check for major overflow */
-	if ((*result - time) / USECS_PER_DAY != date)
-	{
-		*result = 0;			/* keep compiler quiet */
-		return -1;
-	}
-	/* check for just-barely overflow (okay except time-of-day wraps) */
-	/* caution: we want to allow 1999-12-31 24:00:00 */
-	if ((*result < 0 && date > 0) ||
-		(*result > 0 && date < -1))
+	if (unlikely(pg_mul_s64_overflow(date, USECS_PER_DAY, result) ||
+				 pg_add_s64_overflow(*result, time, result)))
 	{
 		*result = 0;			/* keep compiler quiet */
 		return -1;
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..ead74d64d8 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -32,6 +32,12 @@
  * - If a * b overflows, return true, otherwise store the result of a * b
  * into *result. The content of *result is implementation defined in case of
  * overflow.
+ * - If -a overflows, return true, otherwise store the result of -a into
+ * *result. The content of *result is implementation defined in case of
+ * overflow.
+ * - If a is negative, return -a, otherwise return a.  Overflow cannot occur
+ * because the return value is an unsigned integer with the same type as the
+ * argument.
  *---------
  */
 
@@ -97,6 +103,17 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline uint16
+pg_abs_s16(int16 a)
+{
+	/*
+	 * This first widens the argument from int16 to int32 for use with abs().
+	 * The result is then narrowed from int32 to uint16.  This prevents any
+	 * possibility of overflow.
+	 */
+	return (uint16) abs((int32) a);
+}
+
 /*
  * INT32
  */
@@ -154,6 +171,17 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
 #endif
 }
 
+static inline uint32
+pg_abs_s32(int32 a)
+{
+	/*
+	 * This first widens the argument from int32 to int64 for use with
+	 * i64abs().  The result is then narrowed from int64 to uint32.  This
+	 * prevents any possibility of overflow.
+	 */
+	return (uint32) i64abs((int64) a);
+}
+
 /*
  * INT64
  */
@@ -258,6 +286,16 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
 #endif
 }
 
+static inline uint64
+pg_abs_s64(int64 a)
+{
@@ -318,6 +356,24 @@ pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result)
 #endif
 }
 
+static inline bool
@@ -373,6 +429,24 @@ pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result)
 #endif
 }
 
+static inline bool
@@ -438,6 +512,35 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+static inline bool
index f1b143fbd2..402fae6da6 100644
 
+#include "common/int.h"
 #include "dt.h"
 
+	if (unlikely(pg_mul_s64_overflow(dDate, USECS_PER_DAY, result) ||
+				 pg_add_s64_overflow(*result, time, result)))

---

From: Joseph Koshakow <[email-masked]>
Date: Wed, 14 Aug 2024 13:41:40 -0400

Thanks for the improvements Nathan. The current iteration LGTM, just a
single comment on `pg_abs_s64`

> +         return (uint64) PG_INT64_MAX + 1;

Since we know that a does not equal PG_INT64_MIN, could we shorten the
last three lines and do the following?

    static inline uint64
    pg_abs_s64(int64 a)
    {
        if (unlikely(a == PG_INT64_MIN))
          return (uint64) PG_INT64_MAX + 1;
        return i64_abs(a);
    }

Thanks,
Joseph Koshakow

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 14 Aug 2024 13:16:06 -0500

>           return (uint64) PG_INT64_MAX + 1;

Good call.  That actually produces seemingly better code, too.

-- 
nathan
>From 75c4c0e0b180f883ea728f45ffe0a81ac2599dcc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH v22 1/1] Remove dependence on integer wrapping

 src/include/common/int.h                   | 101 +++++++++++++++++++++
 10 files changed, 167 insertions(+), 57 deletions(-)

 	if (value < 0)
 	{

 
 	/*----------
 
 	init_var(&base_prod);
 
+#include "common/int.h"

 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT16_MAX)
 
 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT32_MAX)
 
 	/*
 
 	if (neg)
 	{

 	if (neg)
 	{
 
 	if (tmp > PG_INT64_MAX)

index 7fc046e78a..c76c67e226 100644

+static inline uint16
+pg_abs_s16(int16 a)
+{
+	/*
 
+static inline uint32
+pg_abs_s32(int32 a)
+{
+	/*
@@ -258,6 +286,14 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
 #endif
 }
 
+static inline uint64
+pg_abs_s64(int64 a)
+{
+	if (unlikely(a == PG_INT64_MIN))
+		return (uint64) PG_INT64_MAX + 1;
+	return (uint64) i64abs(a);
+}
+
 /*------------------------------------------------------------------------
  * Overflow routines for unsigned integers
  *------------------------------------------------------------------------
@@ -318,6 +354,24 @@ pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result)
 #endif
 }
 
+static inline bool
@@ -373,6 +427,24 @@ pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result)
 #endif
 }
 
+static inline bool
@@ -438,6 +510,35 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+static inline bool
 
+#include "common/int.h"
 #include "dt.h"

---

From: jian he <[email-masked]>
Date: Thu, 15 Aug 2024 14:56:00 +0800

On Thu, Aug 15, 2024 at 2:16 AM Nathan Bossart <[email-masked]> wrote:

+static inline bool

sorry to bother you.

i am confused with
"
+#elif defined(HAVE_INT128)
+ uint128 res = -((int128) a);
"
I thought "unsigned" means non-negative, therefore uint128 means non-negative.
therefore "int128  res = -((int128) a);" makes sense to me.

also in HAVE_INT128 branch
do we need cast int128 to int64, like

*result = (int64) res;

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 15 Aug 2024 13:45:48 -0500

> therefore "int128  res = -((int128) a);" makes sense to me.

Ah, that's a typo, thanks for pointing it out.

> do we need cast int128 to int64, like

I don't think we need an explicit cast here since *result is known to be an
int64.  But it certainly wouldn't hurt anything...

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 15 Aug 2024 16:34:30 -0500

I've committed 0001.  Now to 0002...

-		if (-element > nelements)
+		if (element == PG_INT32_MIN || -element > nelements)

This seems like a good opportunity to use our new pg_abs_s32() function,
and godbolt.org [0] seems to indicate that it might produce better code,
too (at least to my eye).  I've attached an updated version of the patch
with this change.  Barring additional feedback, I plan to commit this one
shortly.

[0] https://godbolt.org/z/57P4vvGYf

-- 
nathan
>From 62677b351cf18dee37dc0d9253bd694fe6fbf26a Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 6 Jul 2024 15:41:09 -0400
Subject: [PATCH v23 1/1] Remove dependence on integer wrapping for jsonb

This commit updates various jsonb operators and functions to no longer
rely on integer wrapping for correctness. Not all compilers support
-fwrapv, so it's best not to rely on it.
---
 src/backend/utils/adt/jsonfuncs.c   |  7 ++++---
 src/test/regress/expected/jsonb.out | 12 ++++++++++++
 src/test/regress/sql/jsonb.sql      |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 5ecb9fffae..1f8ea51e6a 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -18,6 +18,7 @@
 
 #include "access/htup_details.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/jsonapi.h"
 #include "common/string.h"
 #include "fmgr.h"
@@ -946,7 +947,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
 	{
 		uint32		nelements = JB_ROOT_COUNT(jb);
 
-		if (-element > nelements)
+		if (pg_abs_s32(element) > nelements)
 			PG_RETURN_NULL();
 		else
 			element += nelements;
@@ -5426,7 +5427,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 
 	if (idx < 0)
 	{
-		if (-idx > nelems)
+		if (pg_abs_s32(idx) > nelems)
 		{
 			/*
 			 * If asked to keep elements position consistent, it's not allowed
@@ -5438,7 +5439,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 						 errmsg("path element at position %d is out of range: %d",
 								level + 1, idx)));
 			else
-				idx = INT_MIN;
+				idx = PG_INT32_MIN;
 		}
 		else
 			idx = nelems + idx;
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index e66d760189..a9d93052fc 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -680,6 +680,18 @@ select '"foo"'::jsonb -> 'z';
  
 (1 row)
 
+select '[]'::jsonb -> -2147483648;
+ ?column? 
+----------
+ 
+(1 row)
+
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
+ jsonb_delete_path 
+-------------------
+ {"a": []}
+(1 row)
+
 select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
  ?column? 
 ----------
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 97bc2242a1..6a18577ead 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -204,6 +204,8 @@ select '[{"b": "c"}, {"b": "cc"}]'::jsonb -> 'z';
 select '{"a": "c", "b": null}'::jsonb -> 'b';
 select '"foo"'::jsonb -> 1;
 select '"foo"'::jsonb -> 'z';
+select '[]'::jsonb -> -2147483648;
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
 
 select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
 select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::int;
-- 
2.39.3 (Apple Git-146)

---

From: Joseph Koshakow <[email-masked]>
Date: Thu, 15 Aug 2024 22:49:46 -0400

On Thu, Aug 15, 2024 at 5:34 PM Nathan Bossart <[email-masked]>
wrote:

> Now to 0002...
> too (at least to my eye).

This updated version LGTM, I agree it's a good use of pg_abs_s32().

Thanks,
Joseph Koshakow