On Fri, Aug 30, 2019 at 04:50:21PM +0200, Fabien COELHO wrote: > Given the overheads of the SQL interpreter, I'm unsure about what you would > measure at the SQL level. You could just write a small standalone C program > to test perf and accuracy. Maybe this is what you have in mind.
After a certain threshold, you can see the difference anyway by paying
once the overhead of the function. See for example the updated module
attached that I used for my tests.
I have been testing the various implementations, and doing 2B
iterations leads to roughly the following with a non-assert, -O2
build using mul_u32:
- __builtin_sub_overflow => 5s
- cast to uint64 => 5.9s
- division => 8s
You are right as well that having symmetry with the signed methods is
much better. In order to see the difference, you can just do that
with the extension attached, after of course hijacking int.h with some
undefs and recompiling the backend and the module:
select pg_overflow_check(10000, 10000, 2000000000, 'uint32', 'mul');
>> still it is possible to trick things with signed integer arguments.
>
> Is it?
If you pass -1 and then you can fall back to the maximum of each 16,
32 or 64 bits for the unsigned (see the regression tests I added with
the module).
> Do you mean:
>
> sql> SELECT -32768::INT2;
> ERROR: smallint out of range
You are incorrect here, as the minus sign is ignored by the cast.
This works though:
=# SELECT (-32768)::INT2;
int2
--------
-32768
(1 row)
If you look at int2.sql, we do that:
-- largest and smallest values
INSERT INTO INT2_TBL(f1) VALUES ('32767');
INSERT INTO INT2_TBL(f1) VALUES ('-32767');
That's the part I mean is wrong, as the minimum is actually -32768,
but the test fails to consider that. I'll go fix that after
double-checking other similar tests for int4 and int8.
Attached is an updated patch to complete the work for common/int.h,
with the following changes:
- Changed the multiplication methods for uint16 and uint32 to not be
division-based. uint64 can use that only if int128 exists.
- Added comments on top of each sub-sections for the types checked.
Attached is also an updated version of the module I used to validate
this stuff. Fabien, any thoughts?
--
Michael
overflow.tar.gz
Description: application/gzip
diff --git a/src/include/common/int.h b/src/include/common/int.h
index d754798543..4e1aec5018 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -20,10 +20,28 @@
#ifndef COMMON_INT_H
#define COMMON_INT_H
-/*
- * If a + b overflows, return true, otherwise store the result of a + b into
- * *result. The content of *result is implementation defined in case of
+
+/*---------
+ * The following guidelines apply to all the routines:
+ * - If a + b overflows, return true, otherwise store the result of a + b
+ * into *result. The content of *result is implementation defined in case of
* overflow.
+ * - If a - b overflows, return true, otherwise store the result of a - b
+ * into *result. The content of *result is implementation defined in case of
+ * overflow.
+ * - If a * b overflows, return true, otherwise store the result of a * b
+ * into *result. The content of *result is implementation defined in case of
+ * overflow.
+ *---------
+ */
+
+/*------------------------------------------------------------------------
+ * Overflow routines for signed integers
+ *------------------------------------------------------------------------
+ */
+
+/*
+ * INT16
*/
static inline bool
pg_add_s16_overflow(int16 a, int16 b, int16 *result)
@@ -43,11 +61,6 @@ pg_add_s16_overflow(int16 a, int16 b, int16 *result)
#endif
}
-/*
- * If a - b overflows, return true, otherwise store the result of a - b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_sub_s16_overflow(int16 a, int16 b, int16 *result)
{
@@ -66,11 +79,6 @@ pg_sub_s16_overflow(int16 a, int16 b, int16 *result)
#endif
}
-/*
- * If a * b overflows, return true, otherwise store the result of a * b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
{
@@ -90,9 +98,7 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
}
/*
- * If a + b overflows, return true, otherwise store the result of a + b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
+ * INT32
*/
static inline bool
pg_add_s32_overflow(int32 a, int32 b, int32 *result)
@@ -112,11 +118,6 @@ pg_add_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
-/*
- * If a - b overflows, return true, otherwise store the result of a - b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_sub_s32_overflow(int32 a, int32 b, int32 *result)
{
@@ -135,11 +136,6 @@ pg_sub_s32_overflow(int32 a, int32 b, int32 *result)
#endif
}
-/*
- * If a * b overflows, return true, otherwise store the result of a * b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
{
@@ -159,9 +155,7 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
}
/*
- * If a + b overflows, return true, otherwise store the result of a + b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
+ * INT64
*/
static inline bool
pg_add_s64_overflow(int64 a, int64 b, int64 *result)
@@ -190,11 +184,6 @@ pg_add_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
-/*
- * If a - b overflows, return true, otherwise store the result of a - b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_sub_s64_overflow(int64 a, int64 b, int64 *result)
{
@@ -222,11 +211,6 @@ pg_sub_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
-/*
- * If a * b overflows, return true, otherwise store the result of a * b into
- * *result. The content of *result is implementation defined in case of
- * overflow.
- */
static inline bool
pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
{
@@ -270,4 +254,204 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+/*------------------------------------------------------------------------
+ * Overflow routines for unsigned integers
+ *------------------------------------------------------------------------
+ */
+
+/*
+ * UINT16
+ */
+static inline bool
+pg_add_u16_overflow(uint16 a, uint16 b, uint16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_add_overflow(a, b, result);
+#else
+ uint16 res = a + b;
+
+ if (res < a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
+static inline bool
+pg_sub_u16_overflow(uint16 a, uint16 b, uint16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(a, b, result);
+#else
+ if (b > a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = a - b;
+ return false;
+#endif
+}
+
+static inline bool
+pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_mul_overflow(a, b, result);
+#else
+ uint32 res = (uint32) a * (uint32) b;
+
+ if (res > PG_UINT16_MAX)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = (uint16) res;
+ return false;
+#endif
+}
+
+/*
+ * INT32
+ */
+static inline bool
+pg_add_u32_overflow(uint32 a, uint32 b, uint32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_add_overflow(a, b, result);
+#else
+ uint32 res = a + b;
+
+ if (res < a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
+static inline bool
+pg_sub_u32_overflow(uint32 a, uint32 b, uint32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(a, b, result);
+#else
+ if (b > a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = a - b;
+ return false;
+#endif
+}
+
+static inline bool
+pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_mul_overflow(a, b, result);
+#else
+ uint64 res = (uint64) a * (uint64) b;
+
+ if (res > PG_UINT32_MAX)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = (uint32) res;
+ return false;
+#endif
+}
+
+/*
+ * UINT64
+ */
+static inline bool
+pg_add_u64_overflow(uint64 a, uint64 b, uint64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_add_overflow(a, b, result);
+#elif defined(HAVE_INT128)
+ uint128 res = (uint128) a + (uint128) b;
+
+ if (res > PG_UINT64_MAX)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = (uint64) res;
+ return false;
+#else
+ uint64 res = a + b;
+
+ if (res < a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
+static inline bool
+pg_sub_u64_overflow(uint64 a, uint64 b, uint64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_sub_overflow(a, b, result);
+#elif defined(HAVE_INT128)
+ uint128 res = (uint128) a - (uint128) b;
+
+ if (res > PG_UINT64_MAX)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = (uint64) res;
+ return false;
+#else
+ if (b > a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = a - b;
+ return false;
+#endif
+}
+
+static inline bool
+pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+ return __builtin_mul_overflow(a, b, result);
+#elif defined(HAVE_INT128)
+ uint128 res = (uint128) a * (uint128) b;
+
+ if (res > PG_UINT64_MAX)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = (uint64) res;
+ return false;
+#else
+ uint64 res = a * b;
+
+ if (a != 0 && b != res / a)
+ {
+ *result = 0x5EED; /* to avoid spurious warnings */
+ return true;
+ }
+ *result = res;
+ return false;
+#endif
+}
+
#endif /* COMMON_INT_H */
signature.asc
Description: PGP signature
