[PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist
Hi
Commit 6bf0bc842 replaced float.c's CHECKFLOATVAL() macro with static inline subroutines,
but the fix introduced a performance regression as Tom Lane pointed out and fixed at 607f8ce74.
Found obsolete CHECKFLOATVAL usage in contrib/btree_gist, and tried to fix it according to 607f8ce74.
The attached patch has been testified in master. All tap tests passed.
Regards,
Tang
Attachments:
0001-Remove-obsolete-macro-CHECKFLOATVAL.patchapplication/octet-stream; name=0001-Remove-obsolete-macro-CHECKFLOATVAL.patchDownload
From ae3248f452759228a3080916baa0fdee070ca5dd Mon Sep 17 00:00:00 2001
From: tanghy <tanghy.fnst@fujitsu.com>
Date: Fri, 6 Aug 2021 16:13:05 +0900
Subject: [PATCH] Remove obsolete macro CHECKFLOATVAL
diff --git a/contrib/btree_gist/btree_float4.c b/contrib/btree_gist/btree_float4.c
index 3604c73313..1114d3ca95 100644
--- a/contrib/btree_gist/btree_float4.c
+++ b/contrib/btree_gist/btree_float4.c
@@ -98,7 +98,8 @@ float4_dist(PG_FUNCTION_ARGS)
float4 r;
r = a - b;
- CHECKFLOATVAL(r, isinf(a) || isinf(b), true);
+ if (unlikely(isinf(r)) && !isinf(a) && !isinf(b))
+ float_overflow_error();
PG_RETURN_FLOAT4(Abs(r));
}
diff --git a/contrib/btree_gist/btree_float8.c b/contrib/btree_gist/btree_float8.c
index 10a5262aaa..e5ef193b49 100644
--- a/contrib/btree_gist/btree_float8.c
+++ b/contrib/btree_gist/btree_float8.c
@@ -76,8 +76,8 @@ gbt_float8_dist(const void *a, const void *b, FmgrInfo *flinfo)
float8 r;
r = arg1 - arg2;
- CHECKFLOATVAL(r, isinf(arg1) || isinf(arg2), true);
-
+ if (unlikely(isinf(r)) && !isinf(arg1) && !isinf(arg2))
+ float_overflow_error();
return Abs(r);
}
@@ -106,7 +106,8 @@ float8_dist(PG_FUNCTION_ARGS)
float8 r;
r = a - b;
- CHECKFLOATVAL(r, isinf(a) || isinf(b), true);
+ if (unlikely(isinf(r)) && !isinf(a) && !isinf(b))
+ float_overflow_error();
PG_RETURN_FLOAT8(Abs(r));
}
diff --git a/contrib/btree_gist/btree_utils_num.h b/contrib/btree_gist/btree_utils_num.h
index cec6986172..db7ab54304 100644
--- a/contrib/btree_gist/btree_utils_num.h
+++ b/contrib/btree_gist/btree_utils_num.h
@@ -9,6 +9,7 @@
#include "access/gist.h"
#include "btree_gist.h"
+#include "utils/float.h"
#include "utils/rel.h"
typedef char GBT_NUMKEY;
@@ -89,23 +90,6 @@ typedef struct
#define GET_FLOAT_DISTANCE(t, arg1, arg2) Abs( ((float8) *((const t *) (arg1))) - ((float8) *((const t *) (arg2))) )
-/*
- * check to see if a float4/8 val has underflowed or overflowed
- * borrowed from src/backend/utils/adt/float.c
- */
-#define CHECKFLOATVAL(val, inf_is_valid, zero_is_valid) \
-do { \
- if (isinf(val) && !(inf_is_valid)) \
- ereport(ERROR, \
- (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), \
- errmsg("value out of range: overflow"))); \
- \
- if ((val) == 0.0 && !(zero_is_valid)) \
- ereport(ERROR, \
- (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), \
- errmsg("value out of range: underflow"))); \
-} while(0)
-
extern Interval *abs_interval(Interval *a);
--
2.31.1.windows.1
On Friday, August 6, 2021 11:14 PM, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote:
Commit 6bf0bc842 replaced float.c's CHECKFLOATVAL() macro with static inline subroutines,
but the fix introduced a performance regression as Tom Lane pointed out and fixed at 607f8ce74.Found obsolete CHECKFLOATVAL usage in contrib/btree_gist, and tried to fix it according to 607f8ce74.
Added above patch in commit fest:
https://commitfest.postgresql.org/34/3287/
Regards,
Tang
On Tue, Aug 17, 2021 at 11:08:59AM +0000, tanghy.fnst@fujitsu.com wrote:
On Friday, August 6, 2021 11:14 PM, tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote:
Commit 6bf0bc842 replaced float.c's CHECKFLOATVAL() macro with static inline subroutines,
but the fix introduced a performance regression as Tom Lane pointed out and fixed at 607f8ce74.Found obsolete CHECKFLOATVAL usage in contrib/btree_gist, and tried to fix it according to 607f8ce74.
Added above patch in commit fest:
https://commitfest.postgresql.org/34/3287/
Yes, that does not seem wise on performance grounds. The case of
!zero_is_valid is never reached, so it seems like this code was just a
copy-paste from the float code in the backend. Your patch looks right
to me.
--
Michael
On Wed, Aug 18, 2021 at 10:04:04AM +0900, Michael Paquier wrote:
Yes, that does not seem wise on performance grounds. The case of
!zero_is_valid is never reached, so it seems like this code was just a
copy-paste from the float code in the backend. Your patch looks right
to me.
Applied.
--
Michael
On Thursday, August 19, 2021 12:28 PM, Michael Paquier <michael@paquier.xyz> wrote
On Wed, Aug 18, 2021 at 10:04:04AM +0900, Michael Paquier wrote:
Yes, that does not seem wise on performance grounds. The case of
!zero_is_valid is never reached, so it seems like this code was just a
copy-paste from the float code in the backend. Your patch looks right
to me.Applied.
Thanks for you check and commit, I've changed the patch's commit fest status to 'committed'.
https://commitfest.postgresql.org/34/3287/
Regards,
Tang