Date: Tue, 16 Jul 2024 22:17:54 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Joseph Koshakow <koshy44@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Message-ID: <Zpc34qd6Pv6vbW5O@nathan>
References: 
 <CAAvxfHdhEz4eF7z1wNn_4C4Gkt+e-h7OjzKUJXt7CE+CvubqcQ@mail.gmail.com>
 <CAAvxfHdze21+UZPWo7ePfH19-9hvXe_9SEZk4HABPHuzaUitRQ@mail.gmail.com>
 <CAAvxfHd22ujbY6uTraE+ZSTUN0xX0G-=NZcr0YnKSQuXthcM+g@mail.gmail.com>
 <CAAvxfHcDQEziyQ-ehv0Bg05OovGPGO95VoO7-DyxJx6Tkq85vA@mail.gmail.com>
 <ZpFeopUrhdB5GgNN@nathan>
 <CAAvxfHdX-t03LvxROxbyodBrtzBzZQEKdKB0FCpEr+p1JnFxsA@mail.gmail.com>
 <ZpVAvLbtUPnK1JKe@nathan>
 <CAAvxfHdYnTB7BjkRSPO8__wxB3880TiYD-TVU2uW8q1KbzymPg@mail.gmail.com>
 <Zpa0m_OVw_iWYicV@nathan>
 <CAAvxfHeFDyNZj+rjcuMkh+VTSYmhb_ZPpbdqZUJcjnT=X3ampA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="vjKIKYcf0kqhcu1C"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAvxfHeFDyNZj+rjcuMkh+VTSYmhb_ZPpbdqZUJcjnT=X3ampA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zpc34qd6Pv6vbW5O%40nathan>
Precedence: bulk


--vjKIKYcf0kqhcu1C
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Tue, Jul 16, 2024 at 09:23:27PM -0400, Joseph Koshakow wrote:
> On Tue, Jul 16, 2024 at 1:57 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> The reason I suggested this is so that we could omit all the prerequisite
>> isinf(), isnan(), etc. checks in the cash_mul_float8() and friends.  The
>> checks are slighly different, but from a quick glance it just looks like
> we
>> might end up relying on the FLOAT8_FITS_IN_INT64 check in more cases.
> 
> I don't think we can omit the prerequisite isnan() checks. Neither
> float8_mul() nor float8_div() reject nan inputs/result, and
> FLOAT8_FITS_IN_INT64 has the following to say about inf and nan
> 
>     These macros will do the right thing for Inf, but not necessarily for
> NaN,
>     so check isnan(num) first if that's a possibility.

My instinct would be to just let float8_mul(), float8_div(), etc. handle
the inputs and then to add an additional isnan() check for the result.
This is how it's done elsewhere (e.g., dtoi8() in int8.c).  If something
about the float8 helper functions is inadequate, let's go fix those
instead.

> Though, I think you're right that we can remove the isinf() check from
> cash_mul_float8(). That check is fully covered by FLOAT8_FITS_IN_INT64,
> since all infinite inputs will result in an infinite output. That also
> makes the infinite result check in float8_mul() redundant.
> Additionally, I believe that the underflow check in float8_mul() is
> unnecessary. val1 is an int64 casted to a float8, so it can never be
> -1 < val < 1, so it can never cause an underflow to 0. So I went ahead
> and removed float8_mul() since all of its checks are redundant.

TBH I'd rather keep this stuff super simple by farming out the corner case
handling whenever we can, even if it is redundant in a few cases.  That
keeps it centralized and consistent with the rest of the tree so that any
fixes apply everywhere.  If there's a performance angle, then maybe it's
worth considering the added complexity, though...

> For cash_div_float8() we have a choice. The isinf() input check
> protects against the following, which is not rejected by any of
> the other checks.
> 
>     test=# SELECT '5'::money / 'inf'::float8;
>      ?column?
>     ----------
>         $0.00
>     (1 row)
> 
> For now, I've kept the isinf() input check to reject the above query,
> let me know if you think we should allow this.

We appear to allow this for other data types, so I see no reason to
disallow it for the money type.

I've attached an editorialized version of 0002 based on my thoughts above.

-- 
nathan

--vjKIKYcf0kqhcu1C
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v11-0001-Handle-overflow-in-money-arithmetic.patch"

From a1741b0d9be7a22b078f9d597419aaa1d4aba673 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Thu, 13 Jun 2024 22:39:25 -0400
Subject: [PATCH v11 1/1] Handle overflow in money arithmetic

---
 src/backend/utils/adt/cash.c        | 174 +++++++++++++++-------------
 src/test/regress/expected/money.out | 106 +++++++++++++++++
 src/test/regress/sql/money.sql      |  47 ++++++++
 3 files changed, 247 insertions(+), 80 deletions(-)

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..b20c358486 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -26,6 +26,7 @@
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
 #include "utils/cash.h"
+#include "utils/float.h"
 #include "utils/numeric.h"
 #include "utils/pg_locale.h"
 
@@ -86,6 +87,82 @@ num_word(Cash value)
 	return buf;
 }								/* num_word() */
 
+static inline Cash
+cash_pl_cash(Cash c1, Cash c2)
+{
+	Cash		res;
+
+	if (unlikely(pg_add_s64_overflow(c1, c2, &res)))
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("money out of range")));
+
+	return res;
+}
+
+static inline Cash
+cash_mi_cash(Cash c1, Cash c2)
+{
+	Cash		res;
+
+	if (unlikely(pg_sub_s64_overflow(c1, c2, &res)))
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("money out of range")));
+
+	return res;
+}
+
+static inline Cash
+cash_mul_float8(Cash c, float8 f)
+{
+	float8		res = rint(float8_mul((float8) c, f));
+
+	if (unlikely(isnan(res) || !FLOAT8_FITS_IN_INT64(res)))
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("money out of range")));
+
+	return (Cash) res;
+}
+
+static inline Cash
+cash_div_float8(Cash c, float8 f)
+{
+	float8		res = rint(float8_div((float8) c, f));
+
+	if (unlikely(isnan(res) || !FLOAT8_FITS_IN_INT64(res)))
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("money out of range")));
+
+	return (Cash) res;
+}
+
+static inline Cash
+cash_mul_int64(Cash c, int64 i)
+{
+	Cash		res;
+
+	if (unlikely(pg_mul_s64_overflow(c, i, &res)))
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("money out of range")));
+
+	return res;
+}
+
+static inline Cash
+cash_div_int64(Cash c, int64 i)
+{
+	if (unlikely(i == 0))
+		ereport(ERROR,
+				(errcode(ERRCODE_DIVISION_BY_ZERO),
+				 errmsg("division by zero")));
+
+	return c / i;
+}
+
 /* cash_in()
  * Convert a string to a cash data type.
  * Format is [$]###[,]###[.##]
@@ -612,11 +689,8 @@ cash_pl(PG_FUNCTION_ARGS)
 {
 	Cash		c1 = PG_GETARG_CASH(0);
 	Cash		c2 = PG_GETARG_CASH(1);
-	Cash		result;
-
-	result = c1 + c2;
 
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_pl_cash(c1, c2));
 }
 
 
@@ -628,11 +702,8 @@ cash_mi(PG_FUNCTION_ARGS)
 {
 	Cash		c1 = PG_GETARG_CASH(0);
 	Cash		c2 = PG_GETARG_CASH(1);
-	Cash		result;
-
-	result = c1 - c2;
 
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mi_cash(c1, c2));
 }
 
 
@@ -664,10 +735,8 @@ cash_mul_flt8(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	float8		f = PG_GETARG_FLOAT8(1);
-	Cash		result;
 
-	result = rint(c * f);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_float8(c, f));
 }
 
 
@@ -679,10 +748,8 @@ flt8_mul_cash(PG_FUNCTION_ARGS)
 {
 	float8		f = PG_GETARG_FLOAT8(0);
 	Cash		c = PG_GETARG_CASH(1);
-	Cash		result;
 
-	result = rint(f * c);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_float8(c, f));
 }
 
 
@@ -694,15 +761,8 @@ cash_div_flt8(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	float8		f = PG_GETARG_FLOAT8(1);
-	Cash		result;
 
-	if (f == 0.0)
-		ereport(ERROR,
-				(errcode(ERRCODE_DIVISION_BY_ZERO),
-				 errmsg("division by zero")));
-
-	result = rint(c / f);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_div_float8(c, f));
 }
 
 
@@ -714,10 +774,8 @@ cash_mul_flt4(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	float4		f = PG_GETARG_FLOAT4(1);
-	Cash		result;
 
-	result = rint(c * (float8) f);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_float8(c, (float8) f));
 }
 
 
@@ -729,10 +787,8 @@ flt4_mul_cash(PG_FUNCTION_ARGS)
 {
 	float4		f = PG_GETARG_FLOAT4(0);
 	Cash		c = PG_GETARG_CASH(1);
-	Cash		result;
 
-	result = rint((float8) f * c);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_float8(c, (float8) f));
 }
 
 
@@ -745,15 +801,8 @@ cash_div_flt4(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	float4		f = PG_GETARG_FLOAT4(1);
-	Cash		result;
-
-	if (f == 0.0)
-		ereport(ERROR,
-				(errcode(ERRCODE_DIVISION_BY_ZERO),
-				 errmsg("division by zero")));
 
-	result = rint(c / (float8) f);
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_div_float8(c, (float8) f));
 }
 
 
@@ -765,10 +814,8 @@ cash_mul_int8(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int64		i = PG_GETARG_INT64(1);
-	Cash		result;
 
-	result = c * i;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, i));
 }
 
 
@@ -780,10 +827,8 @@ int8_mul_cash(PG_FUNCTION_ARGS)
 {
 	int64		i = PG_GETARG_INT64(0);
 	Cash		c = PG_GETARG_CASH(1);
-	Cash		result;
 
-	result = i * c;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, i));
 }
 
 /* cash_div_int8()
@@ -794,16 +839,8 @@ cash_div_int8(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int64		i = PG_GETARG_INT64(1);
-	Cash		result;
-
-	if (i == 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_DIVISION_BY_ZERO),
-				 errmsg("division by zero")));
 
-	result = c / i;
-
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_div_int64(c, i));
 }
 
 
@@ -815,10 +852,8 @@ cash_mul_int4(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int32		i = PG_GETARG_INT32(1);
-	Cash		result;
 
-	result = c * i;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, (int64) i));
 }
 
 
@@ -830,10 +865,8 @@ int4_mul_cash(PG_FUNCTION_ARGS)
 {
 	int32		i = PG_GETARG_INT32(0);
 	Cash		c = PG_GETARG_CASH(1);
-	Cash		result;
 
-	result = i * c;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, (int64) i));
 }
 
 
@@ -846,16 +879,8 @@ cash_div_int4(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int32		i = PG_GETARG_INT32(1);
-	Cash		result;
-
-	if (i == 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_DIVISION_BY_ZERO),
-				 errmsg("division by zero")));
-
-	result = c / i;
 
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_div_int64(c, (int64) i));
 }
 
 
@@ -867,10 +892,8 @@ cash_mul_int2(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int16		s = PG_GETARG_INT16(1);
-	Cash		result;
 
-	result = c * s;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, (int64) s));
 }
 
 /* int2_mul_cash()
@@ -881,10 +904,8 @@ int2_mul_cash(PG_FUNCTION_ARGS)
 {
 	int16		s = PG_GETARG_INT16(0);
 	Cash		c = PG_GETARG_CASH(1);
-	Cash		result;
 
-	result = s * c;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_mul_int64(c, (int64) s));
 }
 
 /* cash_div_int2()
@@ -896,15 +917,8 @@ cash_div_int2(PG_FUNCTION_ARGS)
 {
 	Cash		c = PG_GETARG_CASH(0);
 	int16		s = PG_GETARG_INT16(1);
-	Cash		result;
 
-	if (s == 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_DIVISION_BY_ZERO),
-				 errmsg("division by zero")));
-
-	result = c / s;
-	PG_RETURN_CASH(result);
+	PG_RETURN_CASH(cash_div_int64(c, (int64) s));
 }
 
 /* cashlarger()
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out
index 7fd4e31804..890ce1da63 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -528,3 +528,109 @@ SELECT '-92233720368547758.08'::money::numeric;
  -92233720368547758.08
 (1 row)
 
+-- Test overflow checks
+SELECT '92233720368547758.07'::money + '0.01'::money;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money - '0.01'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int8;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::int8;
+ERROR:  money out of range
+SELECT 2::int8 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::int8 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int4;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::int4;
+ERROR:  money out of range
+SELECT 2::int4 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::int4 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::int2;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::int2;
+ERROR:  money out of range
+SELECT 2::int2 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::int2 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::float8;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::float8;
+ERROR:  money out of range
+SELECT 2::float8 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::float8 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '92233720368547758.07'::money * 2::float4;
+ERROR:  money out of range
+SELECT '-92233720368547758.08'::money * 2::float4;
+ERROR:  money out of range
+SELECT 2::float4 * '92233720368547758.07'::money ;
+ERROR:  money out of range
+SELECT 2::float4 * '-92233720368547758.08'::money;
+ERROR:  money out of range
+SELECT '1'::money / 1.175494e-38::float8;
+ERROR:  money out of range
+SELECT '-1'::money / 1.175494e-38::float8;
+ERROR:  money out of range
+SELECT '1'::money / 1.175494e-38::float4;
+ERROR:  money out of range
+SELECT '-1'::money / 1.175494e-38::float4;
+ERROR:  money out of range
+-- Test invalid multipliers/divisors
+SELECT '42'::money * 'inf'::float8;
+ERROR:  money out of range
+SELECT '42'::money * '-inf'::float8;
+ERROR:  money out of range
+SELECT '42'::money * 'nan'::float8;
+ERROR:  money out of range
+SELECT 'inf'::float8 * '42'::money;
+ERROR:  money out of range
+SELECT '-inf'::float8 * '42'::money;
+ERROR:  money out of range
+SELECT 'nan'::float8 * '42'::money;
+ERROR:  money out of range
+SELECT '42'::money / 'inf'::float8;
+ ?column? 
+----------
+    $0.00
+(1 row)
+
+SELECT '42'::money / '-inf'::float8;
+ ?column? 
+----------
+    $0.00
+(1 row)
+
+SELECT '42'::money / 'nan'::float8;
+ERROR:  money out of range
+SELECT '42'::money * 'inf'::float4;
+ERROR:  money out of range
+SELECT '42'::money * '-inf'::float4;
+ERROR:  money out of range
+SELECT '42'::money * 'nan'::float4;
+ERROR:  money out of range
+SELECT 'inf'::float4 * '42'::money;
+ERROR:  money out of range
+SELECT '-inf'::float4 * '42'::money;
+ERROR:  money out of range
+SELECT 'nan'::float4 * '42'::money;
+ERROR:  money out of range
+SELECT '42'::money / 'inf'::float4;
+ ?column? 
+----------
+    $0.00
+(1 row)
+
+SELECT '42'::money / '-inf'::float4;
+ ?column? 
+----------
+    $0.00
+(1 row)
+
+SELECT '42'::money / 'nan'::float4;
+ERROR:  money out of range
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.sql
index 81c92dd960..4422d2816b 100644
--- a/src/test/regress/sql/money.sql
+++ b/src/test/regress/sql/money.sql
@@ -135,3 +135,50 @@ SELECT '12345678901234567'::money::numeric;
 SELECT '-12345678901234567'::money::numeric;
 SELECT '92233720368547758.07'::money::numeric;
 SELECT '-92233720368547758.08'::money::numeric;
+
+-- Test overflow checks
+SELECT '92233720368547758.07'::money + '0.01'::money;
+SELECT '-92233720368547758.08'::money - '0.01'::money;
+SELECT '92233720368547758.07'::money * 2::int8;
+SELECT '-92233720368547758.08'::money * 2::int8;
+SELECT 2::int8 * '92233720368547758.07'::money ;
+SELECT 2::int8 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::int4;
+SELECT '-92233720368547758.08'::money * 2::int4;
+SELECT 2::int4 * '92233720368547758.07'::money ;
+SELECT 2::int4 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::int2;
+SELECT '-92233720368547758.08'::money * 2::int2;
+SELECT 2::int2 * '92233720368547758.07'::money ;
+SELECT 2::int2 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::float8;
+SELECT '-92233720368547758.08'::money * 2::float8;
+SELECT 2::float8 * '92233720368547758.07'::money ;
+SELECT 2::float8 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::float4;
+SELECT '-92233720368547758.08'::money * 2::float4;
+SELECT 2::float4 * '92233720368547758.07'::money ;
+SELECT 2::float4 * '-92233720368547758.08'::money;
+SELECT '1'::money / 1.175494e-38::float8;
+SELECT '-1'::money / 1.175494e-38::float8;
+SELECT '1'::money / 1.175494e-38::float4;
+SELECT '-1'::money / 1.175494e-38::float4;
+-- Test invalid multipliers/divisors
+SELECT '42'::money * 'inf'::float8;
+SELECT '42'::money * '-inf'::float8;
+SELECT '42'::money * 'nan'::float8;
+SELECT 'inf'::float8 * '42'::money;
+SELECT '-inf'::float8 * '42'::money;
+SELECT 'nan'::float8 * '42'::money;
+SELECT '42'::money / 'inf'::float8;
+SELECT '42'::money / '-inf'::float8;
+SELECT '42'::money / 'nan'::float8;
+SELECT '42'::money * 'inf'::float4;
+SELECT '42'::money * '-inf'::float4;
+SELECT '42'::money * 'nan'::float4;
+SELECT 'inf'::float4 * '42'::money;
+SELECT '-inf'::float4 * '42'::money;
+SELECT 'nan'::float4 * '42'::money;
+SELECT '42'::money / 'inf'::float4;
+SELECT '42'::money / '-inf'::float4;
+SELECT '42'::money / 'nan'::float4;
-- 
2.39.3 (Apple Git-146)


--vjKIKYcf0kqhcu1C--