[Tom Lane <tgl@sss.pgh.pa.us>]

I looked a bit more carefully at cash.c in the wake of bug #14663,
https://www.postgresql.org/message-id/20170519164653.29941.19098%40wrigleys.postgresql.org

It seems to me that there are three different bugs in the multiplication
and division operators:

1. As noted in the bug thread, applying rint() to the result of an
integer division is useless, and it will cause precision loss if the
64-bit result exceeds 2^52 or thereabouts.  We should drop it.

2. On the other hand, the cash-times-float operators really should
apply rint() rather than allowing the default truncation behavior to
happen when converting the float product to int64.

3. At least with my compiler (gcc 4.4.7), it seems that arithmetic
between an int64 value and a float4 value is performed by casting
the int64 to float4 and doing the arithmetic in float4.  This results
in really serious precision loss, since the result's only good to
six digits or so.  ISTM we'd be well advised to have the cash-and-float4
operators widen the float4 input to float8 and do the arithmetic in
float8, so that they don't lose more precision than they have to.
On modern machines there's unlikely to be any detectable speed difference.

The attached patch rectifies these things and adds documentation about
the truncation behavior of cash division.  I propose to apply all of
it to HEAD.  What I'm less sure about is how much of it is a candidate
to back-patch.  I think point 1 (precision loss in what should be an
exact integer operation) is a clear bug and we should back-patch it.
But the other cases are not as open-and-shut; maybe we should just
change those behaviors in HEAD.  Thoughts?

			regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachments:

fix-money-precision-issues.patch (text/x-diff, 6.0K) download
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 42b2bb7..a322049 100644
*** a/doc/src/sgml/datatype.sgml
--- b/doc/src/sgml/datatype.sgml
*************** SELECT '52093.89'::money::numeric::float
*** 983,988 ****
--- 983,993 ----
     </para>
  
     <para>
+     Division of a <type>money</type> value by an integer value is performed
+     with truncation of the fractional part towards zero.  To get a rounded
+     result, divide by a floating-point value, or cast the <type>money</type>
+     value to <type>numeric</> before dividing and back to <type>money</type>
+     afterwards.  (The latter is preferable to avoid risking precision loss.)
      When a <type>money</type> value is divided by another <type>money</type>
      value, the result is <type>double precision</type> (i.e., a pure number,
      not money); the currency units cancel each other out in the division.
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 5cb086e..a170294 100644
*** a/src/backend/utils/adt/cash.c
--- b/src/backend/utils/adt/cash.c
*************** cash_mul_flt8(PG_FUNCTION_ARGS)
*** 667,673 ****
  	float8		f = PG_GETARG_FLOAT8(1);
  	Cash		result;
  
! 	result = c * f;
  	PG_RETURN_CASH(result);
  }
  
--- 667,673 ----
  	float8		f = PG_GETARG_FLOAT8(1);
  	Cash		result;
  
! 	result = rint(c * f);
  	PG_RETURN_CASH(result);
  }
  
*************** flt8_mul_cash(PG_FUNCTION_ARGS)
*** 682,688 ****
  	Cash		c = PG_GETARG_CASH(1);
  	Cash		result;
  
! 	result = f * c;
  	PG_RETURN_CASH(result);
  }
  
--- 682,688 ----
  	Cash		c = PG_GETARG_CASH(1);
  	Cash		result;
  
! 	result = rint(f * c);
  	PG_RETURN_CASH(result);
  }
  
*************** cash_mul_flt4(PG_FUNCTION_ARGS)
*** 717,723 ****
  	float4		f = PG_GETARG_FLOAT4(1);
  	Cash		result;
  
! 	result = c * f;
  	PG_RETURN_CASH(result);
  }
  
--- 717,723 ----
  	float4		f = PG_GETARG_FLOAT4(1);
  	Cash		result;
  
! 	result = rint(c * (float8) f);
  	PG_RETURN_CASH(result);
  }
  
*************** flt4_mul_cash(PG_FUNCTION_ARGS)
*** 732,738 ****
  	Cash		c = PG_GETARG_CASH(1);
  	Cash		result;
  
! 	result = f * c;
  	PG_RETURN_CASH(result);
  }
  
--- 732,738 ----
  	Cash		c = PG_GETARG_CASH(1);
  	Cash		result;
  
! 	result = rint((float8) f * c);
  	PG_RETURN_CASH(result);
  }
  
*************** cash_div_flt4(PG_FUNCTION_ARGS)
*** 753,759 ****
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = rint(c / f);
  	PG_RETURN_CASH(result);
  }
  
--- 753,759 ----
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = rint(c / (float8) f);
  	PG_RETURN_CASH(result);
  }
  
*************** cash_div_int8(PG_FUNCTION_ARGS)
*** 802,808 ****
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = rint(c / i);
  
  	PG_RETURN_CASH(result);
  }
--- 802,808 ----
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = c / i;
  
  	PG_RETURN_CASH(result);
  }
*************** cash_div_int4(PG_FUNCTION_ARGS)
*** 854,860 ****
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = rint(c / i);
  
  	PG_RETURN_CASH(result);
  }
--- 854,860 ----
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = c / i;
  
  	PG_RETURN_CASH(result);
  }
*************** cash_div_int2(PG_FUNCTION_ARGS)
*** 904,910 ****
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = rint(c / s);
  	PG_RETURN_CASH(result);
  }
  
--- 904,910 ----
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = c / s;
  	PG_RETURN_CASH(result);
  }
  
diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out
index 0cc69f9..ab86595 100644
*** a/src/test/regress/expected/money.out
--- b/src/test/regress/expected/money.out
*************** SELECT '92233720368547758.075'::money;
*** 359,364 ****
--- 359,414 ----
  ERROR:  value "92233720368547758.075" is out of range for type money
  LINE 1: SELECT '92233720368547758.075'::money;
                 ^
+ -- rounding vs. truncation in division
+ SELECT '878.08'::money / 11::float8;
+  ?column? 
+ ----------
+    $79.83
+ (1 row)
+ 
+ SELECT '878.08'::money / 11::float4;
+  ?column? 
+ ----------
+    $79.83
+ (1 row)
+ 
+ SELECT '878.08'::money / 11::bigint;
+  ?column? 
+ ----------
+    $79.82
+ (1 row)
+ 
+ SELECT '878.08'::money / 11::int;
+  ?column? 
+ ----------
+    $79.82
+ (1 row)
+ 
+ SELECT '878.08'::money / 11::smallint;
+  ?column? 
+ ----------
+    $79.82
+ (1 row)
+ 
+ -- check for precision loss in division
+ SELECT '90000000000000099.00'::money / 10::bigint;
+          ?column?          
+ ---------------------------
+  $9,000,000,000,000,009.90
+ (1 row)
+ 
+ SELECT '90000000000000099.00'::money / 10::int;
+          ?column?          
+ ---------------------------
+  $9,000,000,000,000,009.90
+ (1 row)
+ 
+ SELECT '90000000000000099.00'::money / 10::smallint;
+          ?column?          
+ ---------------------------
+  $9,000,000,000,000,009.90
+ (1 row)
+ 
  -- Cast int4/int8/numeric to money
  SELECT 1234567890::money;
         money       
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.sql
index f5a92f2..37b9ecc 100644
*** a/src/test/regress/sql/money.sql
--- b/src/test/regress/sql/money.sql
*************** SELECT '92233720368547758.08'::money;
*** 97,102 ****
--- 97,114 ----
  SELECT '-92233720368547758.085'::money;
  SELECT '92233720368547758.075'::money;
  
+ -- rounding vs. truncation in division
+ SELECT '878.08'::money / 11::float8;
+ SELECT '878.08'::money / 11::float4;
+ SELECT '878.08'::money / 11::bigint;
+ SELECT '878.08'::money / 11::int;
+ SELECT '878.08'::money / 11::smallint;
+ 
+ -- check for precision loss in division
+ SELECT '90000000000000099.00'::money / 10::bigint;
+ SELECT '90000000000000099.00'::money / 10::int;
+ SELECT '90000000000000099.00'::money / 10::smallint;
+ 
  -- Cast int4/int8/numeric to money
  SELECT 1234567890::money;
  SELECT 12345678901234567::money;