From: Tom Lane <tgl@sss.pgh.pa.us>
To: slawomir.chodnicki@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15925: Loss of precision converting money to numeric
In-reply-to: <15925-da9953e2674bb5c8@postgresql.org>
References: <15925-da9953e2674bb5c8@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Fri, 26 Jul 2019 07:39:17 -0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <12400.1564154154.0@sss.pgh.pa.us>
Date: Fri, 26 Jul 2019 11:17:34 -0400
Message-ID: <12463.1564154254@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <12400.1564154154.1@sss.pgh.pa.us>

PG Bug reporting form <noreply@postgresql.org> writes:
> During my testing I found unexpected results for the min and max value of
> the money type.
> select '-92233720368547758.08'::money,
> '-92233720368547758.08'::money::numeric(30,2);
> money                      |numeric              |
> ---------------------------|---------------------|
> -$92,233,720,368,547,758.08|-92233720368547758.00|
> Note that the cent value is gone after converting to numeric.
> Same issue for the max value:
> money                     |numeric             |
> --------------------------|--------------------|
> $92,233,720,368,547,758.07|92233720368547758.00|

Hmm, yeah, anything approaching INT64_MAX has a problem.
The issue is that cash_numeric() does the equivalent of

SELECT 9223372036854775807::numeric / 100::numeric;

and if you try that by hand you indeed get

 92233720368547758

because select_div_scale() has decided that it need not produce
any fractional digits.  We can force its hand by making the input
have the required number of fractional digits *before* dividing,
which is a bit weird on its face but gets the job done, per the
comment therein:

     * The result scale of a division isn't specified in any SQL standard. For
     * PostgreSQL we select a result scale that will give at least
     * NUMERIC_MIN_SIG_DIGITS significant digits, so that numeric gives a
     * result no less accurate than float8; but use a scale not less than
     * either input's display scale.

(NUMERIC_MIN_SIG_DIGITS is 16, whence the problem for a 17-digit result.
Maybe we should consider raising that, but I'm hesitant to consider such
a far-reaching change just to make cash_numeric happy.)

I intend to apply the attached patch.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-cash_numeric-for-large-values.patch";
	charset="us-ascii"
Content-ID: <12400.1564154154.2@sss.pgh.pa.us>
Content-Description: fix-cash_numeric-for-large-values.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index c92e9d5..800472a 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -1032,13 +1032,8 @@ Datum
 cash_numeric(PG_FUNCTION_ARGS)
 {
 	Cash		money =3D PG_GETARG_CASH(0);
-	Numeric		result;
+	Datum		result;
 	int			fpoint;
-	int64		scale;
-	int			i;
-	Datum		amount;
-	Datum		numeric_scale;
-	Datum		quotient;
 	struct lconv *lconvert =3D PGLC_localeconv();
 =

 	/* see comments about frac_digits in cash_in() */
@@ -1046,22 +1041,45 @@ cash_numeric(PG_FUNCTION_ARGS)
 	if (fpoint < 0 || fpoint > 10)
 		fpoint =3D 2;
 =

-	/* compute required scale factor */
-	scale =3D 1;
-	for (i =3D 0; i < fpoint; i++)
-		scale *=3D 10;
-
 	/* form the result as money / scale */
-	amount =3D DirectFunctionCall1(int8_numeric, Int64GetDatum(money));
-	numeric_scale =3D DirectFunctionCall1(int8_numeric, Int64GetDatum(scale)=
);
-	quotient =3D DirectFunctionCall2(numeric_div, amount, numeric_scale);
+	result =3D DirectFunctionCall1(int8_numeric, Int64GetDatum(money));
+
+	/* Scale appropriately, if needed */
+	if (fpoint > 0)
+	{
+		int64		scale;
+		int			i;
+		Datum		numeric_scale;
+		Datum		quotient;
+
+		/* compute required scale factor */
+		scale =3D 1;
+		for (i =3D 0; i < fpoint; i++)
+			scale *=3D 10;
+		numeric_scale =3D DirectFunctionCall1(int8_numeric,
+											Int64GetDatum(scale));
+
+		/*
+		 * Given integral inputs approaching INT64_MAX, select_div_scale()
+		 * might choose a result scale of zero, causing loss of fractional
+		 * digits in the quotient.  We can ensure an exact result by setting
+		 * the dscale of either input to be at least as large as the desired
+		 * result scale.  numeric_round() will do that for us.
+		 */
+		numeric_scale =3D DirectFunctionCall2(numeric_round,
+											numeric_scale,
+											Int32GetDatum(fpoint));
 =

-	/* forcibly round to exactly the intended number of digits */
-	result =3D DatumGetNumeric(DirectFunctionCall2(numeric_round,
-												 quotient,
-												 Int32GetDatum(fpoint)));
+		/* Now we can safely divide ... */
+		quotient =3D DirectFunctionCall2(numeric_div, result, numeric_scale);
+
+		/* ... and forcibly round to exactly the intended number of digits */
+		result =3D DirectFunctionCall2(numeric_round,
+									 quotient,
+									 Int32GetDatum(fpoint));
+	}
 =

-	PG_RETURN_NUMERIC(result);
+	PG_RETURN_DATUM(result);
 }
 =

 /* numeric_cash()
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expect=
ed/money.out
index ab86595..fc71a72 100644
--- a/src/test/regress/expected/money.out
+++ b/src/test/regress/expected/money.out
@@ -1,6 +1,8 @@
 --
 -- MONEY
 --
+-- Note that we assume lc_monetary has been set to C.
+--
 CREATE TABLE money_data (m money);
 INSERT INTO money_data VALUES ('123');
 SELECT * FROM money_data;
@@ -476,7 +478,7 @@ SELECT (-12345678901234567)::numeric::money;
  -$12,345,678,901,234,567.00
 (1 row)
 =

--- Cast from money
+-- Cast from money to numeric
 SELECT '12345678901234567'::money::numeric;
        numeric        =

 ----------------------
@@ -489,3 +491,15 @@ SELECT '-12345678901234567'::money::numeric;
  -12345678901234567.00
 (1 row)
 =

+SELECT '92233720368547758.07'::money::numeric;
+       numeric        =

+----------------------
+ 92233720368547758.07
+(1 row)
+
+SELECT '-92233720368547758.08'::money::numeric;
+        numeric        =

+-----------------------
+ -92233720368547758.08
+(1 row)
+
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.s=
ql
index 37b9ecc..5e74628 100644
--- a/src/test/regress/sql/money.sql
+++ b/src/test/regress/sql/money.sql
@@ -1,6 +1,8 @@
 --
 -- MONEY
 --
+-- Note that we assume lc_monetary has been set to C.
+--
 =

 CREATE TABLE money_data (m money);
 =

@@ -122,6 +124,8 @@ SELECT (-1234567890)::int4::money;
 SELECT (-12345678901234567)::int8::money;
 SELECT (-12345678901234567)::numeric::money;
 =

--- Cast from money
+-- Cast from money to numeric
 SELECT '12345678901234567'::money::numeric;
 SELECT '-12345678901234567'::money::numeric;
+SELECT '92233720368547758.07'::money::numeric;
+SELECT '-92233720368547758.08'::money::numeric;

------- =_aaaaaaaaaa0--