On Thu, 2 Feb 2023 at 15:15, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Looking at this diff made me wonder why the static pow10[] array > isn't marked "const"? >
Good point. However, looking more closely, I think this function is more or less completely broken: 1). It doesn't work if log10val2 < 0, because then m < 0, and it doesn't multiply by the remainder. And it then throws an overflow error, because result.dscale comes out wrong when m < 0. 2). The result.dscale calculation is wrong if log10val2 is a multiple of DEC_DIGITS, causing it to drop the last 4 digits. 3). If the scaled-up dividend doesn't fit in an int64, the numeric computation breaks if log10val2 >= 10 due to integer overflow. So for example: int64_div_fast_to_numeric(123456, -1) -> ERROR: value overflows numeric format int64_div_fast_to_numeric(123456, 0) -> ERROR: value overflows numeric format int64_div_fast_to_numeric(123456, 4) -> 12 int64_div_fast_to_numeric(123456, 8) -> 0.0012 int64_div_fast_to_numeric(1000000000000000000, 10) -> 709186959.9285992800 As it happens, none of the above represents a live bug, because we currently only call it with log10val2 = 3 or 6, but it's definitely a bug waiting to happen. This was added by a2da77cdb4 ("Change return type of EXTRACT to numeric"), so it only goes back as far as v14. After hacking on it for a while, I ended up with the attached. Regards, Dean
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c new file mode 100644 index 08c8416..6bf6db6 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -4235,7 +4235,7 @@ int64_to_numeric(int64 val) } /* - * Convert val1/(10**val2) to numeric. This is much faster than normal + * Convert val1/(10**log10val2) to numeric. This is much faster than normal * numeric division. */ Numeric @@ -4243,59 +4243,78 @@ int64_div_fast_to_numeric(int64 val1, in { Numeric res; NumericVar result; - int64 saved_val1 = val1; + int rscale; int w; int m; + init_var(&result); + + /* result scale */ + rscale = log10val2 < 0 ? 0 : log10val2; + /* how much to decrease the weight by */ w = log10val2 / DEC_DIGITS; - /* how much is left */ + /* how much is left to divide by */ m = log10val2 % DEC_DIGITS; + if (m < 0) + { + m += DEC_DIGITS; + w--; + } /* - * If there is anything left, multiply the dividend by what's left, then - * shift the weight by one more. + * If there is anything left to divide by (10^m with 0 < m < DEC_DIGITS), + * multiply the dividend by 10^(DEC_DIGITS - m), and shift the weight by + * one more. */ if (m > 0) { #if DEC_DIGITS == 4 - static int pow10[] = {1, 10, 100, 1000}; + static const int pow10[] = {1, 10, 100, 1000}; #elif DEC_DIGITS == 2 - static int pow10[] = {1, 10}; + static const int pow10[] = {1, 10}; #elif DEC_DIGITS == 1 - static int pow10[] = {1}; + static const int pow10[] = {1}; #else #error unsupported NBASE #endif + int64 factor = pow10[DEC_DIGITS - m]; + int64 new_val1; StaticAssertDecl(lengthof(pow10) == DEC_DIGITS, "mismatch with DEC_DIGITS"); - if (unlikely(pg_mul_s64_overflow(val1, pow10[DEC_DIGITS - m], &val1))) + if (unlikely(pg_mul_s64_overflow(val1, factor, &new_val1))) { - /* - * If it doesn't fit, do the whole computation in numeric the slow - * way. Note that va1l may have been overwritten, so use - * saved_val1 instead. - */ - int val2 = 1; +#ifdef HAVE_INT128 + /* do the multiplication using 128-bit integers */ + int128 tmp; - for (int i = 0; i < log10val2; i++) - val2 *= 10; - res = numeric_div_opt_error(int64_to_numeric(saved_val1), int64_to_numeric(val2), NULL); - res = DatumGetNumeric(DirectFunctionCall2(numeric_round, - NumericGetDatum(res), - Int32GetDatum(log10val2))); - return res; + tmp = (int128) val1 * (int128) factor; + + int128_to_numericvar(tmp, &result); +#else + /* do the multiplication using numerics */ + NumericVar tmp; + + init_var(&tmp); + + int64_to_numericvar(val1, &result); + int64_to_numericvar(factor, &tmp); + mul_var(&result, &tmp, &result, 0); + + free_var(&tmp); +#endif } + else + int64_to_numericvar(new_val1, &result); + w++; } - - init_var(&result); - - int64_to_numericvar(val1, &result); + else + int64_to_numericvar(val1, &result); result.weight -= w; - result.dscale += w * DEC_DIGITS - (DEC_DIGITS - m); + result.dscale = rscale; res = make_result(&result);