Keep compiler silence (clang 10, implicit conversion from 'long' to 'double' )
Hello,
I found the problem that clang compiler introduces warnings when building
PostgreSQL. Attached patch fixes it.
===
Compiler version
===
clang version 10.0.0-svn372772-1~exp1+0~20190924181208.2504~1.gbpb209ff
(trunk)
Older versions of clang may not generate this warning.
===
Warning
===
timestamp.c:3236:22: warning: implicit conversion from 'long' to 'double'
changes value from 9223372036854775807 to 9223372036854775808
[-Wimplicit-int-float-conversion]
if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
~ ^~~~~~~~~~~~
../../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
#define INT64CONST(x) (x##L)
^~~~
<scratch space>:234:1: note: expanded from here
0x7FFFFFFFFFFFFFFFL
^~~~~~~~~~~~~~~~~~~
1 warning generated.
pgbench.c:1657:30: warning: implicit conversion from 'long' to 'double'
changes value from 9223372036854775807 to 9223372036854775808
[-Wimplicit-int-float-conversion]
if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
^~~~~~~~~~~~ ~
../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
#define INT64CONST(x) (x##L)
^~~~
<scratch space>:252:1: note: expanded from here
0x7FFFFFFFFFFFFFFFL
^~~~~~~~~~~~~~~~~~~
1 warning generated.
===
This warning is due to implicit conversion from PG_INT64_MAX to double,
which drops the precision as described in the warning. This drop is not a
problem in this case, but we have to get rid of useless warnings. Attached
patch casts PG_INT64_MAX explicitly.
Thanks,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
keep-compiler-silence.patchapplication/octet-stream; name=keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 84bc97d40c..226c9453a9 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,8 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ if (result_double > (double) PG_INT64_MAX
+ || result_double < (double) PG_INT64_MIN)
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ed7652bfbf..4fbc28750c 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1654,7 +1654,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ if (dval < (double) PG_INT64_MIN || (double) PG_INT64_MAX < dval)
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
Hello,
I add further information. This issue also has a problem about
*overflow checking*.
The original code is as follows.
src/backend/utils/adt/timestamp.c:3222
-----
if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
result->time = (int64) result_double;
-----
Here, the code checks whether "result_double" fits 64-bit integer size
before casting it.
However, as I have mentioned in the previous email, PG_INT64_MAX is
cast to double and the value becomes 9223372036854775808 due to lack
of precision.
Therefore, the above code is identical to "result_double >
9223372036854775808.0". This checking does not cover the case when
result_double is equal to 9223372036854775808. In this case, "(int64)
result_double" will be -9223372036854775808, which is wrong.
The next code confirms what I explained.
===
#include <stdio.h>
#include <stdint.h>
int main(void)
{
double value = (double) INT64_MAX;
printf("INT64_MAX = %ld\n", INT64_MAX);
printf("value = %lf\n", value);
printf("(value > (double) INT64_MAX) == %d\n", value > (double) INT64_MAX);
printf("(long int) value == %ld\n", (long int) value);
}
===
Output:
INT64_MAX = 9223372036854775807
value = 9223372036854775808.000000
(value > (double) INT64_MAX) == 0
(long int) value == -9223372036854775808
===
I think the code should be "result_double >= (double) PG_INT64_MAX",
that is we have to use >= rather than >. I attached the modified
patch.
Thanks,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
2019年9月27日(金) 12:00 Yuya Watari <watari.yuya@gmail.com>:
Show quoted text
Hello,
I found the problem that clang compiler introduces warnings when building PostgreSQL. Attached patch fixes it.
===
Compiler version
===
clang version 10.0.0-svn372772-1~exp1+0~20190924181208.2504~1.gbpb209ff (trunk)Older versions of clang may not generate this warning.
===
Warning
===timestamp.c:3236:22: warning: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]
if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
~ ^~~~~~~~~~~~
../../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
#define INT64CONST(x) (x##L)
^~~~
<scratch space>:234:1: note: expanded from here
0x7FFFFFFFFFFFFFFFL
^~~~~~~~~~~~~~~~~~~
1 warning generated.
pgbench.c:1657:30: warning: implicit conversion from 'long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion]
if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
^~~~~~~~~~~~ ~
../../../src/include/c.h:444:22: note: expanded from macro 'PG_INT64_MAX'
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/c.h:381:25: note: expanded from macro 'INT64CONST'
#define INT64CONST(x) (x##L)
^~~~
<scratch space>:252:1: note: expanded from here
0x7FFFFFFFFFFFFFFFL
^~~~~~~~~~~~~~~~~~~
1 warning generated.===
This warning is due to implicit conversion from PG_INT64_MAX to double, which drops the precision as described in the warning. This drop is not a problem in this case, but we have to get rid of useless warnings. Attached patch casts PG_INT64_MAX explicitly.
Thanks,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
v2-keep-compiler-silence.patchapplication/octet-stream; name=v2-keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 84bc97d40c..3ef88cabad 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,13 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ /*
+ * Since PG_INT64_MAX is not representable as a double-precision value, the
+ * cast result of (double) PG_INT64_MAX is 9223372036854775808. Therefore,
+ * we must use >= as a comparison operator when checking overflow.
+ */
+ if (result_double >= (double) PG_INT64_MAX
+ || result_double < (double) PG_INT64_MIN)
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ed7652bfbf..1f1eac5449 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1654,7 +1654,13 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ /*
+ * Since PG_INT64_MAX is not representable as a double-precision value,
+ * the cast result of (double) PG_INT64_MAX is 9223372036854775808.
+ * Therefore, we must use <= as a comparison operator when checking
+ * overflow.
+ */
+ if (dval < (double) PG_INT64_MIN || (double) PG_INT64_MAX <= dval)
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
Hello.
At Fri, 27 Sep 2019 16:43:53 +0900, Yuya Watari <watari.yuya@gmail.com> wrote in <CAJ2pMkaLTOxFjTim=GV8u=jG++sb9W6GNSgyFxPVDSQMVfRv5g@mail.gmail.com>
Hello,
I add further information. This issue also has a problem about
*overflow checking*.The original code is as follows.
src/backend/utils/adt/timestamp.c:3222
-----
if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
result->time = (int64) result_double;
-----I think the code should be "result_double >= (double) PG_INT64_MAX",
that is we have to use >= rather than >. I attached the modified
patch.
Yeah, good catch! It is surely a bogus comparison. But I suspect
that how a number is rounded off depends on architechture. (But
not sure.) There seems be a variant of (double/float)->int32,
say, in interval_div.
I found a trick seems workable generically (*1). (2.0 *
(PG_INT64_MAX/2 + 1)) will generate the value next to the
PG_INT64_MAX based on some assumptions
(*1). IS_DOUBLE_SAFE_IN_INT64() below would be able to check if
the value can be converted into int64 safely or not.
====
/*
* Check if a double value can be casted into int64.
*
* This macro is assuming that FLT_RADIX == 2 so that the * 2.0 trick works,
* PG_INT64_MAX is so below DBL_MAX that the doubled value can be represented
* in double and DBL_MANT_DIG is equal or smaller than DBL_MAX_EXP so that
* ceil() returns expected result.
*/
#define MIN_DOUBLE_OVER_INT64_MAX (2.0 * (PG_INT64_MAX / 2 + 1))
#define MAX_DOUBLE_UNDER_INT64_MIN (2.0 * (PG_INT64_MIN / 2 - 1))
#if -PG_INT64_MAX != PG_INT64_MIN
#define IS_DOUBLE_SAFE_IN_INT64(x) \
((x) < MIN_DOUBLE_OVER_INT64_MAX && ceil(x) >= PG_INT64_MIN)
#else
#define IS_DOUBLE_SAFE_IN_INT64(x) \
((x) < MIN_DOUBLE_OVER_INT64_MAX && (x) > MAX_DOUBLE_UNDER_INT64_MIN)
#endif
====
I haven't fully confirmed if it is really right.
*1: https://stackoverflow.com/questions/526070/handling-overflow-when-casting-doubles-to-integers-in-c
--
Kyotaro Horiguchi
NTT Open Source Software Center
Horiguchi-san,
On Tue, Oct 1, 2019 at 3:41 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
I found a trick seems workable generically (*1). (2.0 *
(PG_INT64_MAX/2 + 1)) will generate the value next to the
PG_INT64_MAX based on some assumptions
(*1). IS_DOUBLE_SAFE_IN_INT64() below would be able to check if
the value can be converted into int64 safely or not.
Thanks for sharing a nice way of checking overflow. I tested your
IS_DOUBLE_SAFE_IN_INT64() macro in my environment by the simple code
(attached to this email) and confirmed that it appropriately handled
the overflow. However, further consideration is needed for different
architectures.
I attached the modified patch. In the patch, I placed the macro in
"src/include/c.h", but this may not be a good choice because c.h is
widely included from a lot of files. Do you have any good ideas about
its placement?
Thanks,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
v3-keep-compiler-silence.patchapplication/octet-stream; name=v3-keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 84bc97d40c..b3bfb21879 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,7 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ if (!IS_DOUBLE_SAFE_IN_INT64(result_double))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ed7652bfbf..6deed64ca4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1654,7 +1654,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ if (!IS_DOUBLE_SAFE_IN_INT64(dval))
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
diff --git a/src/include/c.h b/src/include/c.h
index f461628a24..b70e947ae6 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -444,6 +444,25 @@ typedef unsigned PG_INT128_TYPE uint128
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
#define PG_UINT64_MAX UINT64CONST(0xFFFFFFFFFFFFFFFF)
+/*
+ * Check if a double value can be casted into int64.
+ *
+ * This macro is assuming that FLT_RADIX == 2 so that the * 2.0 trick works,
+ * PG_INT64_MAX is so below DBL_MAX that the doubled value can be represented
+ * in double and DBL_MANT_DIG is equal or smaller than DBL_MAX_EXP so that
+ * ceil() returns expected result.
+ */
+#define MIN_DOUBLE_OVER_INT64_MAX (2.0 * (double) (PG_INT64_MAX / 2 + 1))
+#define MAX_DOUBLE_UNDER_INT64_MIN (2.0 * (double) (PG_INT64_MIN / 2 - 1))
+
+#if -PG_INT64_MAX != PG_INT64_MIN
+#define IS_DOUBLE_SAFE_IN_INT64(x) \
+ ((x) < MIN_DOUBLE_OVER_INT64_MAX && ceil(x) >= (double) PG_INT64_MIN)
+#else
+#define IS_DOUBLE_SAFE_IN_INT64(x) \
+ ((x) < MIN_DOUBLE_OVER_INT64_MAX && (x) > MAX_DOUBLE_UNDER_INT64_MIN)
+#endif
+
/* Max value of size_t might also be missing if we don't have stdint.h */
#ifndef SIZE_MAX
#if SIZEOF_SIZE_T == 8
Yuya Watari <watari.yuya@gmail.com> writes:
I attached the modified patch. In the patch, I placed the macro in
"src/include/c.h", but this may not be a good choice because c.h is
widely included from a lot of files. Do you have any good ideas about
its placement?
I agree that there's an actual bug here; it can be demonstrated with
# select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
date_part
--------------------
-9223372036854.775
(1 row)
which clearly is a wrong answer.
I do not however like any of the proposed patches. We already have one
place that deals with this problem correctly, in int8.c's dtoi8():
/*
* Range check. We must be careful here that the boundary values are
* expressed exactly in the float domain. We expect PG_INT64_MIN to be an
* exact power of 2, so it will be represented exactly; but PG_INT64_MAX
* isn't, and might get rounded off, so avoid using it.
*/
if (unlikely(num < (float8) PG_INT64_MIN ||
num >= -((float8) PG_INT64_MIN) ||
isnan(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
We should adopt that coding technique not invent new ones.
I do concur with creating a macro that encapsulates a correct version
of this test, maybe like
#define DOUBLE_FITS_IN_INT64(num) \
((num) >= (double) PG_INT64_MIN && \
(num) < -((double) PG_INT64_MIN))
(or s/double/float8/ ?)
c.h is probably a reasonable place, seeing that we define the constants
there.
regards, tom lane
At Mon, 04 Nov 2019 12:53:48 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Yuya Watari <watari.yuya@gmail.com> writes:
I attached the modified patch. In the patch, I placed the macro in
"src/include/c.h", but this may not be a good choice because c.h is
widely included from a lot of files. Do you have any good ideas about
its placement?I agree that there's an actual bug here; it can be demonstrated with
# select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
date_part
--------------------
-9223372036854.775
(1 row)which clearly is a wrong answer.
I do not however like any of the proposed patches. We already have one
place that deals with this problem correctly, in int8.c's dtoi8():/*
* Range check. We must be careful here that the boundary values are
* expressed exactly in the float domain. We expect PG_INT64_MIN to be an
* exact power of 2, so it will be represented exactly; but PG_INT64_MAX
* isn't, and might get rounded off, so avoid using it.
*/
if (unlikely(num < (float8) PG_INT64_MIN ||
num >= -((float8) PG_INT64_MIN) ||
isnan(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));We should adopt that coding technique not invent new ones.
I do concur with creating a macro that encapsulates a correct version
of this test, maybe like#define DOUBLE_FITS_IN_INT64(num) \
((num) >= (double) PG_INT64_MIN && \
(num) < -((double) PG_INT64_MIN))
# I didn't noticed the existing bit above.
Agreed. it is equivalent to the trick AFAICS thus no need to add
another one to warry with.
(or s/double/float8/ ?)
Maybe.
c.h is probably a reasonable place, seeing that we define the constants
there.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hello Tom and Horiguchi-san,
On Tue, Nov 5, 2019 at 1:59 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 04 Nov 2019 12:53:48 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in
I do concur with creating a macro that encapsulates a correct version
of this test, maybe like#define DOUBLE_FITS_IN_INT64(num) \
((num) >= (double) PG_INT64_MIN && \
(num) < -((double) PG_INT64_MIN))
Thank you for your comments. The proposed macro "DOUBLE_FITS_IN_INT64"
is a good and simple way to check the overflow. According to that, I
revised the patch, which includes regression tests.
In the patch, I additionally modified other occurrences as follows.
=========
+#define FLOAT8_FITS_IN_INT32(num) \
+ ((num) >= (float8) PG_INT32_MIN && (num) < -((float8) PG_INT32_MIN))=========
- if (unlikely(num < (float8) PG_INT32_MIN ||
- num >= -((float8) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT32(num)))=========
The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
but it sufficiently handles the case.
Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
v4-keep-compiler-silence.patchapplication/octet-stream; name=v4-keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 77a5d7d42f..b9a83683d5 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1212,15 +1212,8 @@ dtoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT32_MIN ||
- num >= -((float8) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT32(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1244,15 +1237,8 @@ dtoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT16_MIN ||
- num >= -((float8) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT16(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
@@ -1300,15 +1286,8 @@ ftoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT32_MIN ||
- num >= -((float4) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT32(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1332,15 +1311,8 @@ ftoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT16_MIN ||
- num >= -((float4) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT16(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 0ff9394a2f..f0f9b97deb 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -1216,15 +1216,8 @@ dtoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT64_MIN ||
- num >= -((float8) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT64(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
@@ -1258,15 +1251,8 @@ ftoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT64_MIN ||
- num >= -((float4) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT64(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 1dc4c820de..8c1572bb2a 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,7 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ if (!FLOAT8_FITS_IN_INT64(result_double))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..3d697cf7f0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1676,7 +1676,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ if (!FLOAT8_FITS_IN_INT64(dval))
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
diff --git a/src/include/c.h b/src/include/c.h
index d752cc07dc..359f2ce937 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -444,6 +444,19 @@ typedef unsigned PG_INT128_TYPE uint128
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
#define PG_UINT64_MAX UINT64CONST(0xFFFFFFFFFFFFFFFF)
+#define FLOAT4_FITS_IN_INT16(num) \
+ ((num) >= (float4) PG_INT16_MIN && (num) < -((float4) PG_INT16_MIN))
+#define FLOAT4_FITS_IN_INT32(num) \
+ ((num) >= (float4) PG_INT32_MIN && (num) < -((float4) PG_INT32_MIN))
+#define FLOAT4_FITS_IN_INT64(num) \
+ ((num) >= (float4) PG_INT64_MIN && (num) < -((float4) PG_INT64_MIN))
+#define FLOAT8_FITS_IN_INT16(num) \
+ ((num) >= (float8) PG_INT16_MIN && (num) < -((float8) PG_INT16_MIN))
+#define FLOAT8_FITS_IN_INT32(num) \
+ ((num) >= (float8) PG_INT32_MIN && (num) < -((float8) PG_INT32_MIN))
+#define FLOAT8_FITS_IN_INT64(num) \
+ ((num) >= (float8) PG_INT64_MIN && (num) < -((float8) PG_INT64_MIN))
+
/* Max value of size_t might also be missing if we don't have stdint.h */
#ifndef SIZE_MAX
#if SIZEOF_SIZE_T == 8
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index f88f34550a..ec81b95606 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -232,6 +232,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
ERROR: interval out of range
LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years'...
^
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+ERROR: interval out of range
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index bc5537d1b9..ddd414e2aa 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -73,6 +73,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483649 days');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 years');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
Yuya Watari <watari.yuya@gmail.com> writes:
The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
but it sufficiently handles the case.
Really? I don't think anything is guaranteed about how a NaN will
compare when using C's non-NaN-aware comparison operators.
My thought about this was to annotate the macros with a reminder
to also check for NaN if there's any possibility that the value
is NaN.
regards, tom lane
Hello Tom,
Thank you for replying.
On Wed, Nov 6, 2019 at 12:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yuya Watari <watari.yuya@gmail.com> writes:
The added macro FLOAT8_FITS_IN_INT32() does not check NaN explicitly,
but it sufficiently handles the case.Really? I don't think anything is guaranteed about how a NaN will
compare when using C's non-NaN-aware comparison operators.My thought about this was to annotate the macros with a reminder
to also check for NaN if there's any possibility that the value
is NaN.
I agree with your opinion. Thank you for pointing it out.
If the platform satisfies IEEE-754 standard, all comparisons (except
for "not equals") between NaN and other floating values are "false".
[1]: https://en.wikipedia.org/wiki/NaN#Comparison_with_NaN
NaN.
[1]: https://en.wikipedia.org/wiki/NaN#Comparison_with_NaN
However, this behavior depends on the platform architecture. As you
have said, C language does not always follow IEEE-754. I think adding
explicit checking of NaN is necessary.
I modified the patch and attached it.
Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
v5-keep-compiler-silence.patchapplication/octet-stream; name=v5-keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 77a5d7d42f..d59db423ea 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1212,15 +1212,8 @@ dtoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT32_MIN ||
- num >= -((float8) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1244,15 +1237,8 @@ dtoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT16_MIN ||
- num >= -((float8) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT16(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
@@ -1300,15 +1286,8 @@ ftoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT32_MIN ||
- num >= -((float4) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT32(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1332,15 +1311,8 @@ ftoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT16_MIN ||
- num >= -((float4) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT16(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 0ff9394a2f..95947b008f 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -1216,15 +1216,8 @@ dtoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT64_MIN ||
- num >= -((float8) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT64(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
@@ -1258,15 +1251,8 @@ ftoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT64_MIN ||
- num >= -((float4) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(!FLOAT4_FITS_IN_INT64(num)) || isnan(num))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 1dc4c820de..8c1572bb2a 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,7 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ if (!FLOAT8_FITS_IN_INT64(result_double))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..3d697cf7f0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1676,7 +1676,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ if (!FLOAT8_FITS_IN_INT64(dval))
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
diff --git a/src/include/c.h b/src/include/c.h
index d752cc07dc..359f2ce937 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -444,6 +444,19 @@ typedef unsigned PG_INT128_TYPE uint128
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
#define PG_UINT64_MAX UINT64CONST(0xFFFFFFFFFFFFFFFF)
+#define FLOAT4_FITS_IN_INT16(num) \
+ ((num) >= (float4) PG_INT16_MIN && (num) < -((float4) PG_INT16_MIN))
+#define FLOAT4_FITS_IN_INT32(num) \
+ ((num) >= (float4) PG_INT32_MIN && (num) < -((float4) PG_INT32_MIN))
+#define FLOAT4_FITS_IN_INT64(num) \
+ ((num) >= (float4) PG_INT64_MIN && (num) < -((float4) PG_INT64_MIN))
+#define FLOAT8_FITS_IN_INT16(num) \
+ ((num) >= (float8) PG_INT16_MIN && (num) < -((float8) PG_INT16_MIN))
+#define FLOAT8_FITS_IN_INT32(num) \
+ ((num) >= (float8) PG_INT32_MIN && (num) < -((float8) PG_INT32_MIN))
+#define FLOAT8_FITS_IN_INT64(num) \
+ ((num) >= (float8) PG_INT64_MIN && (num) < -((float8) PG_INT64_MIN))
+
/* Max value of size_t might also be missing if we don't have stdint.h */
#ifndef SIZE_MAX
#if SIZEOF_SIZE_T == 8
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index f88f34550a..ec81b95606 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -232,6 +232,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
ERROR: interval out of range
LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years'...
^
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+ERROR: interval out of range
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index bc5537d1b9..ddd414e2aa 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -73,6 +73,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483649 days');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 years');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
On Wed, Nov 6, 2019 at 3:33 PM Yuya Watari <watari.yuya@gmail.com> wrote:
However, this behavior depends on the platform architecture. As you
have said, C language does not always follow IEEE-754. I think adding
explicit checking of NaN is necessary.
I'm curious about this point. C may not require IEEE 754 (for
example, on current IBM mainframe and POWER hardware you can opt for
IBM hex floats, and on some IBM platforms that is the default, and the
C compiler isn't breaking any rules by doing that; the only other
floating point format I've heard of is VAX format, long gone, but
perhaps allowed by C). But PostgreSQL effectively requires IEEE 754
since commit 02ddd499322ab6f2f0d58692955dc9633c2150fc, right?
Thomas Munro <thomas.munro@gmail.com> writes:
On Wed, Nov 6, 2019 at 3:33 PM Yuya Watari <watari.yuya@gmail.com> wrote:
However, this behavior depends on the platform architecture. As you
have said, C language does not always follow IEEE-754. I think adding
explicit checking of NaN is necessary.
I'm curious about this point. C may not require IEEE 754 (for
example, on current IBM mainframe and POWER hardware you can opt for
IBM hex floats, and on some IBM platforms that is the default, and the
C compiler isn't breaking any rules by doing that; the only other
floating point format I've heard of is VAX format, long gone, but
perhaps allowed by C). But PostgreSQL effectively requires IEEE 754
since commit 02ddd499322ab6f2f0d58692955dc9633c2150fc, right?
That commit presumes that floats follow the IEEE bitwise representation,
I think; but it's a long way from there to assuming that float comparisons
do something that is explicitly *not* promised by C99. The C spec goes no
further than to state that comparisons on NaNs might raise an exception,
and that's already bad enough. I believe that the assumption Yuya-san was
making about "comparisons on NaNs return false" is only guaranteed by C99
if you use the new-in-C99 macros isless(x, y) and so on, not if you write
x < y.
There's a separate discussion to be had here about whether
!isnan(x) && !isnan(y) && x < y
is more or less efficient, or portable, than
isless(x, y)
but I'm not really in any hurry to start using the latter macros.
regards, tom lane
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
But PostgreSQL effectively requires IEEE 754 since commit
02ddd499322ab6f2f0d58692955dc9633c2150fc, right?
Tom> That commit presumes that floats follow the IEEE bitwise
Tom> representation, I think;
Correct. (It notably does _not_ make any assumptions about how floating
point arithmetic or comparisons work - all the computation is done in
integers.)
Tom> but it's a long way from there to assuming that float comparisons
Tom> do something that is explicitly *not* promised by C99.
I agree.
--
Andrew (irc:RhodiumToad)
Hello Tom, Thomas, and Andrew,
Tom> That commit presumes that floats follow the IEEE bitwise
Tom> representation, I think;Correct. (It notably does _not_ make any assumptions about how floating
point arithmetic or comparisons work - all the computation is done in
integers.)Tom> but it's a long way from there to assuming that float comparisons
Tom> do something that is explicitly *not* promised by C99.I agree.
Thank you for your comments. I agree that we should not assume
anything that is not guaranteed in the language specification. The
modified patch (attached in the previous e-mail) checks NaN explicitly
if needed.
Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
At Wed, 6 Nov 2019 13:56:46 +0900, Yuya Watari <watari.yuya@gmail.com> wrote in
Hello Tom, Thomas, and Andrew,
Tom> That commit presumes that floats follow the IEEE bitwise
Tom> representation, I think;Correct. (It notably does _not_ make any assumptions about how floating
point arithmetic or comparisons work - all the computation is done in
integers.)Tom> but it's a long way from there to assuming that float comparisons
Tom> do something that is explicitly *not* promised by C99.I agree.
Thank you for your comments. I agree that we should not assume
anything that is not guaranteed in the language specification. The
modified patch (attached in the previous e-mail) checks NaN explicitly
if needed.
Mmm? See the bit in the patch cited below (v5).
+ /* Range check */
+ if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))If compiler doesn't any fancy, num is fed to an arithmetic before
checking if it is NaN. That seems have a chance of exception.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hello Horiguchi-san,
On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
Mmm? See the bit in the patch cited below (v5).
+ /* Range check */ + if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))If compiler doesn't any fancy, num is fed to an arithmetic before
checking if it is NaN. That seems have a chance of exception.
Thank you for pointing it out. That's my mistake. I fixed it and
attached the patch.
Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com
Attachments:
v6-keep-compiler-silence.patchapplication/octet-stream; name=v6-keep-compiler-silence.patchDownload
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 77a5d7d42f..47c786f126 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1212,15 +1212,8 @@ dtoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT32_MIN ||
- num >= -((float8) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT8_FITS_IN_INT32(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1244,15 +1237,8 @@ dtoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT16_MIN ||
- num >= -((float8) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT8_FITS_IN_INT16(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
@@ -1300,15 +1286,8 @@ ftoi4(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT32_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT32_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT32_MIN ||
- num >= -((float4) PG_INT32_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT32(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("integer out of range")));
@@ -1332,15 +1311,8 @@ ftoi2(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT16_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT16_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT16_MIN ||
- num >= -((float4) PG_INT16_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT16(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("smallint out of range")));
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index 0ff9394a2f..93acabce42 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -1216,15 +1216,8 @@ dtoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float8) PG_INT64_MIN ||
- num >= -((float8) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT8_FITS_IN_INT64(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
@@ -1258,15 +1251,8 @@ ftoi8(PG_FUNCTION_ARGS)
*/
num = rint(num);
- /*
- * Range check. We must be careful here that the boundary values are
- * expressed exactly in the float domain. We expect PG_INT64_MIN to be an
- * exact power of 2, so it will be represented exactly; but PG_INT64_MAX
- * isn't, and might get rounded off, so avoid using it.
- */
- if (unlikely(num < (float4) PG_INT64_MIN ||
- num >= -((float4) PG_INT64_MIN) ||
- isnan(num)))
+ /* Range check */
+ if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT64(num)))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("bigint out of range")));
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 1dc4c820de..8c1572bb2a 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3233,7 +3233,7 @@ interval_mul(PG_FUNCTION_ARGS)
/* cascade units down */
result->day += (int32) month_remainder_days;
result_double = rint(span->time * factor + sec_remainder * USECS_PER_SEC);
- if (result_double > PG_INT64_MAX || result_double < PG_INT64_MIN)
+ if (!FLOAT8_FITS_IN_INT64(result_double))
ereport(ERROR,
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
errmsg("interval out of range")));
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..3d697cf7f0 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1676,7 +1676,7 @@ coerceToInt(PgBenchValue *pval, int64 *ival)
{
double dval = pval->u.dval;
- if (dval < PG_INT64_MIN || PG_INT64_MAX < dval)
+ if (!FLOAT8_FITS_IN_INT64(dval))
{
fprintf(stderr, "double to int overflow for %f\n", dval);
return false;
diff --git a/src/include/c.h b/src/include/c.h
index d752cc07dc..359f2ce937 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -444,6 +444,19 @@ typedef unsigned PG_INT128_TYPE uint128
#define PG_INT64_MAX INT64CONST(0x7FFFFFFFFFFFFFFF)
#define PG_UINT64_MAX UINT64CONST(0xFFFFFFFFFFFFFFFF)
+#define FLOAT4_FITS_IN_INT16(num) \
+ ((num) >= (float4) PG_INT16_MIN && (num) < -((float4) PG_INT16_MIN))
+#define FLOAT4_FITS_IN_INT32(num) \
+ ((num) >= (float4) PG_INT32_MIN && (num) < -((float4) PG_INT32_MIN))
+#define FLOAT4_FITS_IN_INT64(num) \
+ ((num) >= (float4) PG_INT64_MIN && (num) < -((float4) PG_INT64_MIN))
+#define FLOAT8_FITS_IN_INT16(num) \
+ ((num) >= (float8) PG_INT16_MIN && (num) < -((float8) PG_INT16_MIN))
+#define FLOAT8_FITS_IN_INT32(num) \
+ ((num) >= (float8) PG_INT32_MIN && (num) < -((float8) PG_INT32_MIN))
+#define FLOAT8_FITS_IN_INT64(num) \
+ ((num) >= (float8) PG_INT64_MIN && (num) < -((float8) PG_INT64_MIN))
+
/* Max value of size_t might also be missing if we don't have stdint.h */
#ifndef SIZE_MAX
#if SIZEOF_SIZE_T == 8
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index f88f34550a..ec81b95606 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -232,6 +232,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
ERROR: interval out of range
LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years'...
^
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+ERROR: interval out of range
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index bc5537d1b9..ddd414e2aa 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -73,6 +73,10 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483649 days');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 years');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
+-- Test an interval whose epoch is not representable as 64 bits
+-- The next should fail as out-of-range
+select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
+
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
Yuya Watari <watari.yuya@gmail.com> writes:
On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:+ if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
If compiler doesn't any fancy, num is fed to an arithmetic before
checking if it is NaN. That seems have a chance of exception.
Thank you for pointing it out. That's my mistake. I fixed it and
attached the patch.
Actually, that mistake is very old --- the existing functions tested
isnan() last for a long time. I agree that testing isnan() first
is safer, but it seems that the behavior of throwing an exception
for comparisons on NaN is rarer than one might guess from the C spec.
Another issue in the patch as it stands is that the FITS_IN_ macros
require the input to have already been rounded with rint(), else they'll
give the wrong answer for values just a bit smaller than -PG_INTnn_MIN.
The existing uses of the technique did that, and interval_mul already
did too, but I had to adjust pgbench. This is largely a documentation
failure: not only did you fail to add any commentary about the new macros,
but you removed most of the commentary that had been in-line in the
existing usages.
I fixed those things and pushed it.
regards, tom lane
Hello Tom,
Thank you for your comments.
On Fri, Nov 8, 2019 at 1:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
failure: not only did you fail to add any commentary about the new macros,
but you removed most of the commentary that had been in-line in the
existing usages.
I apologize for the insufficient comments. I had to add more
information about these macros.
I fixed those things and pushed it.
Thank you very much for the commit!
Best regards,
Yuya Watari
NTT Software Innovation Center
watari.yuya@gmail.com