On Fri, 6 Aug 2021 at 03:58, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rash...@gmail.com> writes:
> > On Thu, 5 Aug 2021 at 17:04, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> It looks like castoroides is not happy with this patch:
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=castoroides&dt=2021-08-01%2008%3A52%3A43
>
> > Hmm, there's something very weird going on there.
>
> Yeah. I tried to reproduce this on the gcc compile farm's Solaris 10
> machine, but the test passed fine for me. The only obvious configuration
> difference I can find is that that machine has
>
> $ cc -V
> cc: Sun C 5.10 SunOS_sparc Patch 141861-10 2012/11/07
>
> whereas castorides' compiler seems to be a few years older. So this
> does seem like it could be a compiler bug.
>
Ah, so the latest test results from castoroides confirm my previous
hypothesis, that it isn't even reaching the new code in power_var():
0.9999999999 ^ 23300000000000 returned 1.0199545627709647
0.9999999999 ^ 70000000000000 returned 0.9396000441558118
which are actually the results you'd get if you just cast the exponent
to an int32, throwing away the top 32 bits and compute the results:
0.9999999999 ^ -197580800 = 1.0199545627709647
0.9999999999 ^ 623009792 = 0.9396000441558118
So the "test for overflow by reverse-conversion" obviously isn't
working as intended, and it's going through power_var_int() when it
shouldn't. I don't think there's anything wrong with that code, so I
think this is a compiler bug.
I guess the best thing to do is just test the value against
PG_INT32_MIN/MAX, which is what int84() does. There are 2 other places
in numeric.c that use similar code to check for int16/32 overflow, so
it's possible that they're broken in the same way on that platform,
but they aren't covered by the regression tests, so it's also possible
that they're OK. Anyway, something like the attached seems likely to
be safer.
Regards,
Dean
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
new file mode 100644
index 13bb968..0c210a3
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -4289,11 +4289,13 @@ numericvar_to_int32(const NumericVar *va
if (!numericvar_to_int64(var, &val))
return false;
+ if (unlikely(val < PG_INT32_MIN) || unlikely(val > PG_INT32_MAX))
+ return false;
+
/* Down-convert to int4 */
*result = (int32) val;
- /* Test for overflow by reverse-conversion. */
- return ((int64) *result == val);
+ return true;
}
Datum
@@ -4373,15 +4375,14 @@ numeric_int2(PG_FUNCTION_ARGS)
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
- /* Down-convert to int2 */
- result = (int16) val;
-
- /* Test for overflow by reverse-conversion. */
- if ((int64) result != val)
+ if (unlikely(val < PG_INT16_MIN) || unlikely(val > PG_INT16_MAX))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
+ /* Down-convert to int2 */
+ result = (int16) val;
+
PG_RETURN_INT16(result);
}
@@ -10186,10 +10187,7 @@ power_var(const NumericVar *base, const
if (numericvar_to_int64(exp, &expval64))
{
- int expval = (int) expval64;
-
- /* Test for overflow by reverse-conversion. */
- if ((int64) expval == expval64)
+ if (expval64 >= PG_INT32_MIN && expval64 <= PG_INT32_MAX)
{
/* Okay, select rscale */
rscale = NUMERIC_MIN_SIG_DIGITS;
@@ -10197,7 +10195,7 @@ power_var(const NumericVar *base, const
rscale = Max(rscale, NUMERIC_MIN_DISPLAY_SCALE);
rscale = Min(rscale, NUMERIC_MAX_DISPLAY_SCALE);
- power_var_int(base, expval, result, rscale);
+ power_var_int(base, (int) expval64, result, rscale);
return;
}
}
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
new file mode 100644
index 80b42f8..3707aff
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -1154,6 +1154,55 @@ SELECT * FROM fract_only;
(7 rows)
DROP TABLE fract_only;
+-- Check conversion to integers
+SELECT (-9223372036854775808.5)::int8; -- should fail
+ERROR: bigint out of range
+SELECT (-9223372036854775808.4)::int8; -- ok
+ int8
+----------------------
+ -9223372036854775808
+(1 row)
+
+SELECT 9223372036854775807.4::int8; -- ok
+ int8
+---------------------
+ 9223372036854775807
+(1 row)
+
+SELECT 9223372036854775807.5::int8; -- should fail
+ERROR: bigint out of range
+SELECT (-2147483648.5)::int4; -- should fail
+ERROR: integer out of range
+SELECT (-2147483648.4)::int4; -- ok
+ int4
+-------------
+ -2147483648
+(1 row)
+
+SELECT 2147483647.4::int4; -- ok
+ int4
+------------
+ 2147483647
+(1 row)
+
+SELECT 2147483647.5::int4; -- should fail
+ERROR: integer out of range
+SELECT (-32768.5)::int2; -- should fail
+ERROR: smallint out of range
+SELECT (-32768.4)::int2; -- ok
+ int2
+--------
+ -32768
+(1 row)
+
+SELECT 32767.4::int2; -- ok
+ int2
+-------
+ 32767
+(1 row)
+
+SELECT 32767.5::int2; -- should fail
+ERROR: smallint out of range
-- Check inf/nan conversion behavior
SELECT 'NaN'::float8::numeric;
numeric
diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
new file mode 100644
index e643752..60d2a71
--- a/src/test/regress/sql/numeric.sql
+++ b/src/test/regress/sql/numeric.sql
@@ -773,6 +773,20 @@ INSERT INTO fract_only VALUES (11, '-Inf
SELECT * FROM fract_only;
DROP TABLE fract_only;
+-- Check conversion to integers
+SELECT (-9223372036854775808.5)::int8; -- should fail
+SELECT (-9223372036854775808.4)::int8; -- ok
+SELECT 9223372036854775807.4::int8; -- ok
+SELECT 9223372036854775807.5::int8; -- should fail
+SELECT (-2147483648.5)::int4; -- should fail
+SELECT (-2147483648.4)::int4; -- ok
+SELECT 2147483647.4::int4; -- ok
+SELECT 2147483647.5::int4; -- should fail
+SELECT (-32768.5)::int2; -- should fail
+SELECT (-32768.4)::int2; -- ok
+SELECT 32767.4::int2; -- ok
+SELECT 32767.5::int2; -- should fail
+
-- Check inf/nan conversion behavior
SELECT 'NaN'::float8::numeric;
SELECT 'Infinity'::float8::numeric;
