On Thu, 2 Feb 2023 at 15:15, Tom Lane 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(100, 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 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], )))
+ if (unlikely(pg_mul_s64_overflow(val1, factor, _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, );
+#else
+ /* do the multiplication using numerics */
+ NumericVar tmp;
+
+ init_var();
+
+ int64_to_numericvar(val1, );
+ int64_to_numericvar(factor, );
+ mul_var(, , , 0);
+
+ free_var();
+#endif
}
+ else
+ int64_to_numericvar(new_val1, );
+
w++;
}
-
- init_var();
-
- int64_to_numericvar(val1, );
+ else
+ int64_to_numericvar(val1, );
result.weight -= w;
- result.dscale += w * DEC_DIGITS - (DEC_DIGITS - m);
+ result.dscale = rscale;
res = make_result();
