[PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist

Started by tanghy.fnst@fujitsu.comover 4 years ago5 messages
#1tanghy.fnst@fujitsu.com
tanghy.fnst@fujitsu.com
1 attachment(s)

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

#2tanghy.fnst@fujitsu.com
tanghy.fnst@fujitsu.com
In reply to: tanghy.fnst@fujitsu.com (#1)
RE: [PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist

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

#3Michael Paquier
michael@paquier.xyz
In reply to: tanghy.fnst@fujitsu.com (#2)
Re: [PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist

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

#4Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#3)
Re: [PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist

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

#5tanghy.fnst@fujitsu.com
tanghy.fnst@fujitsu.com
In reply to: Michael Paquier (#4)
RE: [PATCH]Remove obsolete macro CHECKFLOATVAL in btree_gist

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