Re: [HACKERS] pow support for pgbench

[Robert Haas]

On Tue, Dec 26, 2017 at 4:45 PM, Raúl Marín Rodríguez
 wrote:
> I've implemented the overflow checks and made some benchmarks and the ipow()
> version became slower except with some specific inputs (base 0 for example).
> It's true that the new auxiliary functions could be optimized, but I don't
> think it makes sense to keep working on them just to match pow() speed.
>
> I'm attaching both patches in case someone wants to have a look but I would
> go with the simpler solution (pgbench_pow_v10.patch).

Committed the simpler solution after fixing it so that it compiles.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Bonjour Michaël,


And my 2c on the matter is that switching silently from one version to
the other would be unwelcome. The user should be aware if a test is
overflowing a number when specifying an integer.


This whole integer pow version is becoming unduly complicated and ugly.

For me, the rational of having the ipow implementation was to have a 
precise integer result when possible. Now that it is casted to double and 
the precision is lost, the whole point of ipow is moot, even if there is 
some performance gain.


So given that committers do not want the int/double version because it is 
slightly different from the numeric/double version of SQL (obviously), and 
that the integer version is becoming over complicated with checks and 
warnings or errors, I'm now in favor of just dropping it, and provide the 
double version only.


Too bad for integer computations on keys which is the core of pgbench use 
case, but I can't help it.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Tue, Dec 26, 2017 at 11:26:58PM +0100, Fabien COELHO wrote:
> > This version looks good to me, except that I wonder if we should try to
> > switch to the floating-point version if the integer version would/does
> > overflow.
> 
> My 0.02€ is that it is under the user control who provides either ints or
> doubles as arguments. So I do not think that we should bother, for what my
> opinion is worth.
> 
> If this is a new requirement, detecting the integer overflow is probably
> possible with some testing, eg unexpected changes of sign, but that would
> probably add two tests per round, and probably double the computation cost.

And my 2c on the matter is that switching silently from one version to
the other would be unwelcome. The user should be aware if a test is
overflowing a number when specifying an integer.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi,

I've implemented the overflow checks and made some benchmarks and the
ipow() version became slower except with some specific inputs (base 0 for
example). It's true that the new auxiliary functions could be optimized,
but I don't think it makes sense to keep working on them just to match
pow() speed.

I'm attaching both patches in case someone wants to have a look but I would
go with the simpler solution (pgbench_pow_v10.patch).

Regards,
-- 

*Raúl Marín Rodríguez *carto.com
From 28bc71f657fa967bafc03c015e5e2405c427a711 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Thu, 21 Dec 2017 20:28:02 +0100
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 +++
 src/bin/pgbench/exprparse.y  |  6 ++
 src/bin/pgbench/pgbench.c| 16 
 src/bin/pgbench/pgbench.h|  3 ++-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 22 +-
 5 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3eb7..1519fe78ef 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1069,6 +1069,13 @@ pgbench  options  d
pi()
3.14159265358979323846
   
+  
+   pow(x, y), power(x, y)
+   double
+   exponentiation
+   pow(2.0, 10), power(2.0, 10)
+   1024.0
+  
   
random(lb, ub)
integer
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 25d5ad48e5..74ffe5e7a7 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -194,6 +194,12 @@ static const struct
 	{
 		"random_zipfian", 3, PGBENCH_RANDOM_ZIPFIAN
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
+	{
+		"power", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7ce6f607f5..5b444765dd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1850,6 +1850,22 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+
+Assert(nargs == 2);
+
+if (!coerceToDouble(lval, ) ||
+	!coerceToDouble(rval, ))
+	return false;
+
+setDoubleValue(retval, pow(ld, rd));
+
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index 83fee1ae74..0e92882a4c 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -76,7 +76,8 @@ typedef enum PgBenchFunction
 	PGBENCH_RANDOM,
 	PGBENCH_RANDOM_GAUSSIAN,
 	PGBENCH_RANDOM_EXPONENTIAL,
-	PGBENCH_RANDOM_ZIPFIAN
+	PGBENCH_RANDOM_ZIPFIAN,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index e3cdf28628..9cbeb2fc11 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -232,7 +232,17 @@ pgbench(
 		qr{command=19.: double 19\b},
 		qr{command=20.: double 20\b},
 		qr{command=21.: int 9223372036854775807\b},
-		qr{command=23.: int [1-9]\b}, ],
+		qr{command=23.: int [1-9]\b},
+		qr{command=24.: double -27\b},
+		qr{command=25.: double 1024\b},
+		qr{command=26.: double 1\b},
+		qr{command=27.: double 1\b},
+		qr{command=28.: double -0.125\b},
+		qr{command=29.: double -0.125\b},
+		qr{command=30.: double -0.00032\b},
+		qr{command=31.: double 8.50705917302346e\+37\b},
+		qr{command=32.: double 1e\+30\b},
+	],
 	'pgbench expressions',
 	{   '001_pgbench_expressions' => q{-- integer functions
 \set i1 debug(random(1, 100))
@@ -264,6 +274,16 @@ pgbench(
 \set i1 0
 -- yet another integer function
 \set id debug(random_zipfian(1, 9, 1.3))
+--- pow and power
+\set poweri debug(pow(-3,3))
+\set powerd debug(pow(2.0,10))
+\set poweriz debug(pow(0,0))
+\set powerdz debug(pow(0.0,0.0))
+\set powernegi debug(pow(-2,-3))
+\set powernegd debug(pow(-2.0,-3.0))
+\set powernegd2 debug(power(-5.0,-5.0))
+\set powerov debug(pow(9223372036854775807, 2))
+\set powerov2 debug(pow(10,30))
 } });
 
 # backslash commands
-- 
2.15.1

From 6cb8fb1a3eaffe680b85913b0acf0068dcb0d2a7 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Thu, 21 Dec 2017 20:28:02 +0100
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|   7 ++
 src/bin/pgbench/exprparse.y  |   6 ++
 src/bin/pgbench/pgbench.c| 100 +++
 src/bin/pgbench/pgbench.h|   3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl |  22 +-
 5 files changed, 136 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3eb7..1519fe78ef 100644
--- 

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Robert,


If a double is always returned, I'm wondering whether keeping the ipow
version makes much sense: In case of double loss of precision, the precision
is lost, too bad, and casting back to int won't bring it back.


I've kept it because knowing that both are ints enables not making a lot of
checks (done in math.h pow) so it's way faster. In my system it's 2-3ns vs
~40ns. I'm willing to settle for using just pow() if that makes it clearer.


This version looks good to me, except that I wonder if we should try to 
switch to the floating-point version if the integer version would/does 
overflow.


My 0.02€ is that it is under the user control who provides either ints or 
doubles as arguments. So I do not think that we should bother, for what my 
opinion is worth.


If this is a new requirement, detecting the integer overflow is probably 
possible with some testing, eg unexpected changes of sign, but that would 
probably add two tests per round, and probably double the computation 
cost.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Robert Haas]

On Fri, Dec 22, 2017 at 12:46 AM, Raúl Marín Rodríguez
 wrote:
>> If a double is always returned, I'm wondering whether keeping the ipow
>> version makes much sense: In case of double loss of precision, the precision
>> is lost, too bad, and casting back to int won't bring it back.
>
> I've kept it because knowing that both are ints enables not making a lot of
> checks (done in math.h pow) so it's way faster. In my system it's 2-3ns vs
> ~40ns. I'm willing to settle for using just pow() if that makes it clearer.

This version looks good to me, except that I wonder if we should try
to switch to the floating-point version if the integer version
would/does overflow.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello,


If a double is always returned, I'm wondering whether keeping the ipow
version makes much sense: In case of double loss of precision, the
precision is lost, too bad, and casting back to int won't bring it back.


I've kept it because knowing that both are ints enables not making a lot of
checks (done in math.h pow) so it's way faster. In my system it's 2-3ns vs
~40ns. I'm willing to settle for using just pow() if that makes it clearer.


Ok, performance is a good argument. I would not have thought that the 
double performance would be so bad, but probably no miracle.


As of precision, there is another case where the int computation 
overflows, so that the int result is stupid and the double version is a 
better approximation. Now that can be controled by providing double or int 
arguments to the function, so for me it is ok.



Attached the  updated patch.


Ok.

I have marked it as ready to committer.

Basically for me this is an inferior version, not specially behaving that 
better with respect to SQL, but if it eventually gets through a committer 
maybe it is worth it.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi Fabien,

Thanks for the review.

If a double is always returned, I'm wondering whether keeping the ipow
> version makes much sense: In case of double loss of precision, the
> precision is lost, too bad, and casting back to int won't bring it back.

I've kept it because knowing that both are ints enables not making a lot of
checks (done in math.h pow) so it's way faster. In my system it's 2-3ns vs
~40ns. I'm willing to settle for using just pow() if that makes it clearer.

In the doc, I'm not sure that "Numeric" brings anything. "Exponentiation"
> would be enough.

Done.

Also, in pg I just noticed that POW is a shorthand for POWER. Maybe both
> should be supported? Or not.

I've never used power instead of pow, but I've added for compatibility
shake.

Attached the  updated patch.

On Thu, Dec 21, 2017 at 10:48 PM, Fabien COELHO  wrote:

>
> Hello Raúl,
>
> v7 needs a rebase.
>>>
>>> Also, you might try to produce a version which is compatible with
>>> Robert's
>>> constraints.
>>>
>>
> My 0.02€ on this new version: Applies cleanly, compiles and works.
>
> I cannot say that I like it more than the previous version.
>
> If a double is always returned, I'm wondering whether keeping the ipow
> version makes much sense: In case of double loss of precision, the
> precision is lost, too bad, and casting back to int won't bring it back.
>
> In the doc, I'm not sure that "Numeric" brings anything. "Exponentiation"
> would be enough.
>
> Also, in pg I just noticed that POW is a shorthand for POWER. Maybe both
> should be supported? Or not.
>
> --
> Fabien.




-- 

*Raúl Marín Rodríguez *carto.com
From 58463d2751c016a68410d5357625bf3d7f519ca2 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Thu, 21 Dec 2017 20:28:02 +0100
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 
 src/bin/pgbench/exprparse.y  |  6 +++
 src/bin/pgbench/pgbench.c| 60 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 18 -
 5 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3eb7..1519fe78ef 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1069,6 +1069,13 @@ pgbench  options  d
pi()
3.14159265358979323846
   
+  
+   pow(x, y), power(x, y)
+   double
+   exponentiation
+   pow(2.0, 10), power(2.0, 10)
+   1024.0
+  
   
random(lb, ub)
integer
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 25d5ad48e5..74ffe5e7a7 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -194,6 +194,12 @@ static const struct
 	{
 		"random_zipfian", 3, PGBENCH_RANDOM_ZIPFIAN
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
+	{
+		"power", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7ce6f607f5..f1c1e568e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -911,6 +911,25 @@ getZipfianRand(TState *thread, int64 min, int64 max, double s)
 	  : computeHarmonicZipfian(thread, n, s));
 }
 
+/* Faster power function for integer values and exp >= 0 */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result = 1;
+
+	Assert(exp >= 0);
+
+	while (exp)
+	{
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
 /*
  * Initialize the given SimpleStats struct to all zeroes
  */
@@ -1850,6 +1869,47 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+
+Assert(nargs == 2);
+
+/*
+ * If both operands are int and exp >= 0 use
+ * the faster ipow() function, else use pow()
+ */
+if (lval->type == PGBT_INT &&
+	 rval->type == PGBT_INT)
+{
+
+	int64		li,
+ri;
+
+	if (!coerceToInt(lval, ) ||
+		!coerceToInt(rval, ))
+		return false;
+
+	if (ri >= 0)
+		setDoubleValue(retval, ipow(li, ri));
+	else
+		setDoubleValue(retval, pow(li, ri));
+}
+else
+{
+	double		ld,
+rd;
+
+	if (!coerceToDouble(lval, ) ||
+		!coerceToDouble(rval, ))
+		return false;
+
+	setDoubleValue(retval, pow(ld, rd));
+}
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index 83fee1ae74..0e92882a4c 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -76,7 +76,8 @@ typedef enum PgBenchFunction
 	PGBENCH_RANDOM,
 	PGBENCH_RANDOM_GAUSSIAN,
 	PGBENCH_RANDOM_EXPONENTIAL,
-	PGBENCH_RANDOM_ZIPFIAN
+	PGBENCH_RANDOM_ZIPFIAN,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct 

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Raúl,


v7 needs a rebase.

Also, you might try to produce a version which is compatible with Robert's
constraints.


My 0.02€ on this new version: Applies cleanly, compiles and works.

I cannot say that I like it more than the previous version.

If a double is always returned, I'm wondering whether keeping the ipow 
version makes much sense: In case of double loss of precision, the 
precision is lost, too bad, and casting back to int won't bring it back.


In the doc, I'm not sure that "Numeric" brings anything. "Exponentiation" 
would be enough.


Also, in pg I just noticed that POW is a shorthand for POWER. Maybe both 
should be supported? Or not.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi,

Rebased the patch on top of current master/HEAD and changed to always
return double.

On Thu, Dec 14, 2017 at 12:48 PM, Fabien COELHO  wrote:

>
> Fixed in the attached patch.
>>
>
> v7 needs a rebase.
>
> Also, you might try to produce a version which is compatible with Robert's
> constraints.
>
> --
> Fabien.
>



-- 

*Raúl Marín Rodríguez *carto.com
From 0b43116dc68cbe46d5f5caa9ba10f36cda188949 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Thu, 21 Dec 2017 20:28:02 +0100
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 
 src/bin/pgbench/exprparse.y  |  3 ++
 src/bin/pgbench/pgbench.c| 60 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 16 +++-
 5 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3eb7..6f7c640db0 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1069,6 +1069,13 @@ pgbench  options  d
pi()
3.14159265358979323846
   
+  
+   pow(x, y)
+   double
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0
+  
   
random(lb, ub)
integer
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 25d5ad48e5..89e1a3b14f 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -194,6 +194,9 @@ static const struct
 	{
 		"random_zipfian", 3, PGBENCH_RANDOM_ZIPFIAN
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7ce6f607f5..f1c1e568e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -911,6 +911,25 @@ getZipfianRand(TState *thread, int64 min, int64 max, double s)
 	  : computeHarmonicZipfian(thread, n, s));
 }
 
+/* Faster power function for integer values and exp >= 0 */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result = 1;
+
+	Assert(exp >= 0);
+
+	while (exp)
+	{
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
 /*
  * Initialize the given SimpleStats struct to all zeroes
  */
@@ -1850,6 +1869,47 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+
+Assert(nargs == 2);
+
+/*
+ * If both operands are int and exp >= 0 use
+ * the faster ipow() function, else use pow()
+ */
+if (lval->type == PGBT_INT &&
+	 rval->type == PGBT_INT)
+{
+
+	int64		li,
+ri;
+
+	if (!coerceToInt(lval, ) ||
+		!coerceToInt(rval, ))
+		return false;
+
+	if (ri >= 0)
+		setDoubleValue(retval, ipow(li, ri));
+	else
+		setDoubleValue(retval, pow(li, ri));
+}
+else
+{
+	double		ld,
+rd;
+
+	if (!coerceToDouble(lval, ) ||
+		!coerceToDouble(rval, ))
+		return false;
+
+	setDoubleValue(retval, pow(ld, rd));
+}
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index 83fee1ae74..0e92882a4c 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -76,7 +76,8 @@ typedef enum PgBenchFunction
 	PGBENCH_RANDOM,
 	PGBENCH_RANDOM_GAUSSIAN,
 	PGBENCH_RANDOM_EXPONENTIAL,
-	PGBENCH_RANDOM_ZIPFIAN
+	PGBENCH_RANDOM_ZIPFIAN,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index e3cdf28628..031b97fc54 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -232,7 +232,14 @@ pgbench(
 		qr{command=19.: double 19\b},
 		qr{command=20.: double 20\b},
 		qr{command=21.: int 9223372036854775807\b},
-		qr{command=23.: int [1-9]\b}, ],
+		qr{command=23.: int [1-9]\b},
+		qr{command=24.: double -27\b},
+		qr{command=25.: double 1024\b},
+		qr{command=26.: double 1\b},
+		qr{command=27.: double 1\b},
+		qr{command=28.: double -0.125\b},
+		qr{command=29.: double -0.125\b},
+	],
 	'pgbench expressions',
 	{   '001_pgbench_expressions' => q{-- integer functions
 \set i1 debug(random(1, 100))
@@ -264,6 +271,13 @@ pgbench(
 \set i1 0
 -- yet another integer function
 \set id debug(random_zipfian(1, 9, 1.3))
+--- pow() operator
+\set poweri debug(pow(-3,3))
+\set powerd debug(pow(2.0,10))
+\set poweriz debug(pow(0,0))
+\set powerdz debug(pow(0.0,0.0))
+\set powernegi debug(pow(-2,-3))
+\set powernegd debug(pow(-2.0,-3.0))
 } });
 
 # backslash commands
-- 
2.15.1

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Fixed in the attached patch.


v7 needs a rebase.

Also, you might try to produce a version which is compatible with 
Robert's constraints.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

In both cases we'd return a double but we use the fast ipow if it's 
possible (which can be 20x faster), so at the cost of an extra cast if 
you need an int, we'd have a consistent API. Would this be acceptable?


It seems OK to me.


Computing as an int, casting to double and back to int8 can generate a 
loss of precision. However for powers of 2 it works exactly, so eg 
computing a mask it would be ok.


This proposal does not exactly match SQL behavior, but I do not see this 
as a problem, which is why I was happy with the previous proposal.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Robert Haas]

On Tue, Dec 5, 2017 at 7:44 AM, Raúl Marín Rodríguez
 wrote:
> I've been giving a thought about this and I think we could reach the
> compromise
> of having a single function with 2 overloads:
> * pow(double, double) -> double: Uses C pow().
> * pow(int, int) -> double: Uses ipow() for positive exponents, and pow()
> for negative exponents.
>
> In both cases we'd return a double but we use the fast ipow if it's possible
> (which can be 20x faster), so at the cost of an extra cast if you need an
> int,
> we'd have a consistent API. Would this be acceptable?

It seems OK to me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

I've been giving a thought about this and I think we could reach the
compromise
of having a single function with 2 overloads:
* pow(double, double) -> double: Uses C pow().
* pow(int, int) -> double: Uses ipow() for positive exponents, and pow()
for negative exponents.

In both cases we'd return a double but we use the fast ipow if it's 
possible (which can be 20x faster), so at the cost of an extra cast if 
you need an int, we'd have a consistent API. Would this be acceptable?


This is for Robert to say whether it is more acceptable to him.

My 0.02€: ISTM that it closely equivalent to having just the double 
version and using an explicit cast to get an int if needed, which does not 
conform anymore to strict SQL behavior than the previous compromise.


Also, probably having something (anything) is better than nothing.

--
Fabien.

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi all,

I've been giving a thought about this and I think we could reach the
compromise
of having a single function with 2 overloads:
* pow(double, double) -> double: Uses C pow().
* pow(int, int) -> double: Uses ipow() for positive exponents, and pow()
for negative exponents.

In both cases we'd return a double but we use the fast ipow if it's possible
(which can be 20x faster), so at the cost of an extra cast if you need an
int,
we'd have a consistent API. Would this be acceptable?

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Tom,


1. A patch that adds an integer version of pow() but not a double version
2. A patch that adds a double version of pow() but not an integer version
3. A patch that adds both an integer version of pow() and a double
version of pow(), with the two versions having different names



It seems to me that 1 and 2 have value on their own for the workloads
tried to be emulated, so what you are suggesting in 3 looks good to
me. Now why are two different function names necessary?


ISTM one key issue here is whether pgbench's expression language is
meant to model SQL (where we have function overloading) or C (where
there is no overloading).  I don't think we've really settled on a
fixed policy on that, but maybe now is the time.


Function overloading is implemented for ABS (as noted by Michaël), but 
also LEAST and GREATEST. Typing is dynamic, based on guessing (think 
"pgbench -D i=1 -D f=1.0") . These decisions have already been taken, they 
were reasonable, the behavior is consistent and useful.


I do not see the point of going backward and breaking compatibility.

The idea is to model after SQL (eg also I've been asked to do that for the 
operators & functions extensions, lost in the CF queue), when possible.

When possible is not always.


If we do think that function overloading is OK, there remains the
question of when the typing is resolved.


Dynamic guessing is the only pragmatic option with pgbench.

I think Robert is objecting to resolving at runtime, and I tend to agree 
that that's something we'd regret in the long run.


Too late. I do not think that we would come to regret it. I'm rather 
regretting that pgbench capabilities are improving so slowly.



It doesn't match either SQL or C.


Sure. A dynamically typed language cannot match a statically typed one. I 
do not see this as a significant issue for writing benchmarking scripts.



Now, about POW:

As for approximating NUMERIC, there is a first difficulty, as the type can 
either be approximated as an INT or DOUBLE. Ah ah.


Also, POW raises another difficulty, which is that depending on the sign 
of the second argument the result is more INT or more DOUBLE. Fun.


So even if we do separate functions (IPOW & DPOW, or POW & DPOW), the 
result cannot be typed statically.


Note that type can be enforced if necessary thanks to int() and double() 
functions, which is enough of a work around for pgbench simple purpose.


In this context, ISTM that Raul patch is a reasonable, even if imperfect, 
compromise.



I can understand that this compromise does not suit Robert. Too bad, 
because both POW versions (int & double) have a reasonable use case for 
writing benchmarks. Using two names does not resolve the typing issue 
anyway. None of the 3 options offered by Robert are really satisfactory, 
and the compromise is also rejected.


So basically do whatever you want with the patch (accept, reject, 1, 2, 
3). Only "Returned with feedback" with the feedback being "please 
implement a statically typed pgbench" does not seem like a useful option.


I can only say that these functions would be useful, so for me it is 
better in than out, but that is just my silly opinion.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Tue, Dec 5, 2017 at 11:32 AM, Tom Lane  wrote:
> ISTM one key issue here is whether pgbench's expression language is
> meant to model SQL (where we have function overloading) or C (where
> there is no overloading).  I don't think we've really settled on a
> fixed policy on that, but maybe now is the time.

abs() is doing that already. Having some rules in the shape of at
least a comment would be nice.

> If we do think that function overloading is OK, there remains the
> question of when the typing is resolved.  I think Robert is objecting
> to resolving at runtime, and I tend to agree that that's something
> we'd regret in the long run.  It doesn't match either SQL or C.

+1.
-- 
Michael

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Tue, Dec 5, 2017 at 5:38 AM, Robert Haas  wrote:
> I'm willing to commit any of the following things:
>
> 1. A patch that adds an integer version of pow() but not a double version
> 2. A patch that adds a double version of pow() but not an integer version
> 3. A patch that adds both an integer version of pow() and a double
> version of pow(), with the two versions having different names
>
> If Raúl is happy with only having an integer version, then I suggest
> that he adopt #1 and call it good.  Otherwise, given that Fabien wants
> the double version, I suggest we call the integer version pow() and
> the double version dpow() and go with #3.

It seems to me that 1 and 2 have value on their own for the workloads
tried to be emulated, so what you are suggesting in 3 looks good to
me. Now why are two different function names necessary? The parsing
takes care of argument types through PgBenchValue->type so having one
function exposed to the user looks like the most sensible approach to
me.
-- 
Michael

Re: [HACKERS] pow support for pgbench

[Robert Haas]

On Mon, Dec 4, 2017 at 10:47 AM, Fabien COELHO  wrote:
>> What's the name of the backend function whose behavior this matches?
>>
>> As Fabien has mentioned, it tries to behave as "numeric_power". Maybe we
>> it'd better if we switch to "dpow" (which is pow with some error handling)
>> and always return a double. What do you think?
>
> My 0.02€: I think that having a integer pow implementation when possible is
> a good think for pgbench, because the main use case is to deal with table
> keys in a benchmarking scripts, which are expected to be integers.

I'm willing to commit any of the following things:

1. A patch that adds an integer version of pow() but not a double version
2. A patch that adds a double version of pow() but not an integer version
3. A patch that adds both an integer version of pow() and a double
version of pow(), with the two versions having different names

If Raúl is happy with only having an integer version, then I suggest
that he adopt #1 and call it good.  Otherwise, given that Fabien wants
the double version, I suggest we call the integer version pow() and
the double version dpow() and go with #3.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Please add the new function into the documentation table in 
alphabetical order.


Fixed in the attached patch.


Yep. Patch applies cleanly. Make check & pgbench check ok. make html ok. 
POW is in the right place in the table, sorry I did not check before.



What's the name of the backend function whose behavior this matches?

As Fabien has mentioned, it tries to behave as "numeric_power". Maybe we 
it'd better if we switch to "dpow" (which is pow with some error 
handling) and always return a double. What do you think?


My 0.02€: I think that having a integer pow implementation when possible 
is a good think for pgbench, because the main use case is to deal with 
table keys in a benchmarking scripts, which are expected to be integers.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

The fact that the return type is not consistently of one type bothers
me.  I'm not sure pgbench's expression language is a good place to
runtime polymorphism -- SQL doesn't work that way.


Sure.

Pg has a NUMERIC adaptative precision version, which is cheating, because it 
can return kind of an "int" or a "float", depending on whether there are 
digits after the decimal point or not.


Pgbench does not have support for NUMERIC, just INT & DOUBLE, so the current 
version is an approximation of that.


Now it is always possible to just do DOUBLE version, but this won't match SQL 
behavior either.


Another point I forgot: pgbench functions and operators are notably 
interesting to generate/transform keys in tables, which are usually 
integers, so having int functions when possible/appropriate is desirable, 
which explain why I pushed for having an int version for POW.


Also, pgbench does not have a static typing model because variable types 
are not declared "\set i ...", so the type is somehow "guessed" based on 
the string values, although if in doubt it is always possible to convert 
(with "int" & "double" functions).


So for me the philosophy is to have expression match SQL behavior when 
possible, as closely as possible, but it is not an exact match.


--
Fabien.

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Robert,


The fact that the return type is not consistently of one type bothers
me.  I'm not sure pgbench's expression language is a good place to
runtime polymorphism -- SQL doesn't work that way.


Sure.

Pg has a NUMERIC adaptative precision version, which is cheating, because 
it can return kind of an "int" or a "float", depending on whether there 
are digits after the decimal point or not.


Pgbench does not have support for NUMERIC, just INT & DOUBLE, so the 
current version is an approximation of that.


Now it is always possible to just do DOUBLE version, but this won't match 
SQL behavior either.



+ /*
+  * pow() for integer values with exp >= 0. Matches SQL pow() behaviour
+  */

What's the name of the backend function whose behavior this matches?


POW(numeric,numeric) -> numeric, which matches "numeric_power".

--
Fabien.

Re: [HACKERS] pow support for pgbench

[Robert Haas]

On Fri, Dec 1, 2017 at 4:57 AM, Raúl Marín Rodríguez
 wrote:
> I've rebased the patch so it can be applied cleanly on top of current
> master.

Please add the new function into the documentation table in alphabetical order.

The fact that the return type is not consistently of one type bothers
me.  I'm not sure pgbench's expression language is a good place to
runtime polymorphism -- SQL doesn't work that way.

+ /*
+  * pow() for integer values with exp >= 0. Matches SQL pow() behaviour
+  */

What's the name of the backend function whose behavior this matches?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi Fabien,

The idea is that it would be relative to the "more functions and operators"
> patch, but I guess this is too much for people checking, so I'm fine with
> having it with the current base.
>

I tried applying the last "more functions and operators" patch
(pgbench-more-ops-funcs-14.patch) but it also stopped applying cleanly so I
decided to go for master to avoid having too many issues. Let me know if
you rework that patch and I'll be happy to rebase mine on top.


-- 

*Raúl Marín Rodríguez *carto.com

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi,

I've rebased the patch so it can be applied cleanly on top of current
master.

On Fri, Dec 1, 2017 at 3:55 AM, Michael Paquier 
wrote:

> On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO  wrote:
> > Let's now hope that a committer gets around to consider these patch some
> > day.
>
> Which is not the case yet, so moved to CF 2018-01. Please note that
> the patch proposed does not apply anymore, so its status is changed to
> "waiting on author" for a rebase.
> --
> Michael
>



-- 

*Raúl Marín Rodríguez *carto.com
From b12a7a37b1af1ede1aa9d5d88d0918808c54e19f Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Fri, 1 Dec 2017 10:49:17 +0100
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 
 src/bin/pgbench/exprparse.y  |  3 ++
 src/bin/pgbench/pgbench.c| 62 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 16 ++-
 5 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 94b495e606..769e604dd6 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1099,6 +1099,13 @@ pgbench  options  d
sqrt(2.0)
1.414213562
   
+  
+   pow(x, y)
+   integer if x and y are integers and y >= 0, else double
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0
+  
  
  

diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index b3a2d9bfd3..4db3b215ab 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -191,6 +191,9 @@ static const struct
 	{
 		"random_exponential", 3, PGBENCH_RANDOM_EXPONENTIAL
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bd96eae5e6..5a5197cf63 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -746,6 +746,27 @@ getPoissonRand(TState *thread, int64 center)
 	return (int64) (-log(uniform) * ((double) center) + 0.5);
 }
 
+ /*
+  * pow() for integer values with exp >= 0. Matches SQL pow() behaviour
+  */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result = 1;
+
+	Assert(exp >= 0);
+
+	while (exp)
+	{
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
 /*
  * Initialize the given SimpleStats struct to all zeroes
  */
@@ -1673,6 +1694,47 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+
+Assert(nargs == 2);
+
+/*
+ * If both operands are int and exp >= 0 use
+ * the ipow() function, else use pow()
+ */
+if (lval->type == PGBT_INT &&
+	 rval->type == PGBT_INT)
+{
+
+	int64		li,
+ri;
+
+	if (!coerceToInt(lval, ) ||
+		!coerceToInt(rval, ))
+		return false;
+
+	if (ri >= 0)
+		setIntValue(retval, ipow(li, ri));
+	else
+		setDoubleValue(retval, pow(li, ri));
+}
+else
+{
+	double		ld,
+rd;
+
+	if (!coerceToDouble(lval, ) ||
+		!coerceToDouble(rval, ))
+		return false;
+
+	setDoubleValue(retval, pow(ld, rd));
+}
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index fd428af274..e0132b5fcf 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -75,7 +75,8 @@ typedef enum PgBenchFunction
 	PGBENCH_SQRT,
 	PGBENCH_RANDOM,
 	PGBENCH_RANDOM_GAUSSIAN,
-	PGBENCH_RANDOM_EXPONENTIAL
+	PGBENCH_RANDOM_EXPONENTIAL,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index c095881312..fcb30cdde5 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -231,7 +231,14 @@ pgbench(
 		qr{command=18.: double 18\b},
 		qr{command=19.: double 19\b},
 		qr{command=20.: double 20\b},
-		qr{command=21.: int 9223372036854775807\b}, ],
+		qr{command=21.: int 9223372036854775807\b},
+		qr{command=23.: int -27\b},
+		qr{command=24.: double 1024\b},
+		qr{command=25.: int 1\b},
+		qr{command=26.: double 1\b},
+		qr{command=27.: double -0.125\b},
+		qr{command=28.: double -0.125\b},
+],
 	'pgbench expressions',
 	{   '001_pgbench_expressions' => q{-- integer functions
 \set i1 debug(random(1, 100))
@@ -261,6 +268,13 @@ pgbench(
 \set maxint debug(:minint - 1)
 -- reset a variable
 \set i1 0
+--- pow() operator
+\set poweri debug(pow(-3,3))
+\set powerd debug(pow(2.0,10))
+\set poweriz debug(pow(0,0))
+\set powerdz debug(pow(0.0,0.0))
+\set powernegi debug(pow(-2,-3))
+\set 

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO  wrote:
> Let's now hope that a committer gets around to consider these patch some
> day.

Which is not the case yet, so moved to CF 2018-01. Please note that
the patch proposed does not apply anymore, so its status is changed to
"waiting on author" for a rebase.
-- 
Michael