Avoid gratuitous inaccuracy in numeric width_bucket().

Multiply before dividing, not the reverse, so that cases that should
produce exact results do produce exact results.  (width_bucket_float8
got this right already.)  Even when the result is inexact, this avoids
making it more inexact, since only the division step introduces any
imprecision.

While at it, fix compute_bucket() to not uselessly repeat the sign
check already done by its caller, and avoid duplicating the
multiply/divide steps by adjusting variable usage.

Per complaint from Martin Visser.  Although this seems like a bug fix,
I'm hesitant to risk changing width_bucket()'s results in stable
branches, so no back-patch.

Discussion: https://postgr.es/m/6FA5117D-6AED-4656-8FEF-B74AC18FAD85@brytlyt.com

diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index a4692d8cfc059f9c21c4a89b536788e20b7c3b35..20c9cac2fa2e08929599b5beb7905e43bdcf6544 100644 (file)
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -592,7 +592,8 @@ static void round_var(NumericVar *var, int rscale);
 static void trunc_var(NumericVar *var, int rscale);
 static void strip_var(NumericVar *var);
 static void compute_bucket(Numeric operand, Numeric bound1, Numeric bound2,
-                          const NumericVar *count_var, NumericVar *result_var);
+                          const NumericVar *count_var, bool reversed_bounds,
+                          NumericVar *result_var);
 
 static void accum_sum_add(NumericSumAccum *accum, const NumericVar *var1);
 static void accum_sum_rescale(NumericSumAccum *accum, const NumericVar *val);
@@ -1752,8 +1753,8 @@ width_bucket_numeric(PG_FUNCTION_ARGS)
            else if (cmp_numerics(operand, bound2) >= 0)
                add_var(&count_var, &const_one, &result_var);
            else
-               compute_bucket(operand, bound1, bound2,
-                              &count_var, &result_var);
+               compute_bucket(operand, bound1, bound2, &count_var, false,
+                              &result_var);
            break;
 
            /* bound1 > bound2 */
@@ -1763,8 +1764,8 @@ width_bucket_numeric(PG_FUNCTION_ARGS)
            else if (cmp_numerics(operand, bound2) <= 0)
                add_var(&count_var, &const_one, &result_var);
            else
-               compute_bucket(operand, bound1, bound2,
-                              &count_var, &result_var);
+               compute_bucket(operand, bound1, bound2, &count_var, true,
+                              &result_var);
            break;
    }
 
@@ -1783,11 +1784,13 @@ width_bucket_numeric(PG_FUNCTION_ARGS)
 /*
  * If 'operand' is not outside the bucket range, determine the correct
  * bucket for it to go. The calculations performed by this function
- * are derived directly from the SQL2003 spec.
+ * are derived directly from the SQL2003 spec. Note however that we
+ * multiply by count before dividing, to avoid unnecessary roundoff error.
  */
 static void
 compute_bucket(Numeric operand, Numeric bound1, Numeric bound2,
-              const NumericVar *count_var, NumericVar *result_var)
+              const NumericVar *count_var, bool reversed_bounds,
+              NumericVar *result_var)
 {
    NumericVar  bound1_var;
    NumericVar  bound2_var;
@@ -1797,23 +1800,21 @@ compute_bucket(Numeric operand, Numeric bound1, Numeric bound2,
    init_var_from_num(bound2, &bound2_var);
    init_var_from_num(operand, &operand_var);
 
-   if (cmp_var(&bound1_var, &bound2_var) < 0)
+   if (!reversed_bounds)
    {
        sub_var(&operand_var, &bound1_var, &operand_var);
        sub_var(&bound2_var, &bound1_var, &bound2_var);
-       div_var(&operand_var, &bound2_var, result_var,
-               select_div_scale(&operand_var, &bound2_var), true);
    }
    else
    {
        sub_var(&bound1_var, &operand_var, &operand_var);
-       sub_var(&bound1_var, &bound2_var, &bound1_var);
-       div_var(&operand_var, &bound1_var, result_var,
-               select_div_scale(&operand_var, &bound1_var), true);
+       sub_var(&bound1_var, &bound2_var, &bound2_var);
    }
 
-   mul_var(result_var, count_var, result_var,
-           result_var->dscale + count_var->dscale);
+   mul_var(&operand_var, count_var, &operand_var,
+           operand_var.dscale + count_var->dscale);
+   div_var(&operand_var, &bound2_var, result_var,
+           select_div_scale(&operand_var, &bound2_var), true);
    add_var(result_var, &const_one, result_var);
    floor_var(result_var, result_var);
 

diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
index 70d6cfe4232c6c4245a1e9e2469e567ce8003ec1..cb782d0e2a8f2e0c76735da90e5f0c6485991888 100644 (file)
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -1385,6 +1385,46 @@ SELECT width_bucket('Infinity'::float8, 1, 10, 10),
 (1 row)
 
 DROP TABLE width_bucket_test;
+-- Simple test for roundoff error when results should be exact
+SELECT x, width_bucket(x::float8, 10, 100, 9) as flt,
+       width_bucket(x::numeric, 10, 100, 9) as num
+FROM generate_series(0, 110, 10) x;
+  x  | flt | num 
+-----+-----+-----
+   0 |   0 |   0
+  10 |   1 |   1
+  20 |   2 |   2
+  30 |   3 |   3
+  40 |   4 |   4
+  50 |   5 |   5
+  60 |   6 |   6
+  70 |   7 |   7
+  80 |   8 |   8
+  90 |   9 |   9
+ 100 |  10 |  10
+ 110 |  10 |  10
+(12 rows)
+
+SELECT x, width_bucket(x::float8, 100, 10, 9) as flt,
+       width_bucket(x::numeric, 100, 10, 9) as num
+FROM generate_series(0, 110, 10) x;
+  x  | flt | num 
+-----+-----+-----
+   0 |  10 |  10
+  10 |  10 |  10
+  20 |   9 |   9
+  30 |   8 |   8
+  40 |   7 |   7
+  50 |   6 |   6
+  60 |   5 |   5
+  70 |   4 |   4
+  80 |   3 |   3
+  90 |   2 |   2
+ 100 |   1 |   1
+ 110 |   0 |   0
+(12 rows)
+
+--
 -- TO_CHAR()
 --
 SELECT '' AS to_char_1, to_char(val, '9G999G999G999G999G999')

diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
index 520e23a9f554c09fb7a6323da5c4b3f12f36aad7..76969db22a7d1fc48557a8d2d259814561879e1f 100644 (file)
--- a/src/test/regress/sql/numeric.sql
+++ b/src/test/regress/sql/numeric.sql
@@ -888,6 +888,15 @@ SELECT width_bucket('Infinity'::float8, 1, 10, 10),
 
 DROP TABLE width_bucket_test;
 
+-- Simple test for roundoff error when results should be exact
+SELECT x, width_bucket(x::float8, 10, 100, 9) as flt,
+       width_bucket(x::numeric, 10, 100, 9) as num
+FROM generate_series(0, 110, 10) x;
+SELECT x, width_bucket(x::float8, 100, 10, 9) as flt,
+       width_bucket(x::numeric, 100, 10, 9) as num
+FROM generate_series(0, 110, 10) x;
+
+--
 -- TO_CHAR()
 --
 SELECT '' AS to_char_1, to_char(val, '9G999G999G999G999G999')