From 7538708394e7a70105a4e601e253adf80f47cca8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 8 Oct 2020 13:06:27 -0400 Subject: 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 --- src/backend/utils/adt/numeric.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) (limited to 'src/backend/utils/adt/numeric.c') diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index a4692d8cfc0..20c9cac2fa2 100644 --- 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); -- cgit v1.2.3