pow support for pgbench

On Fri, Oct 27, 2017 at 4:51 PM, Raúl Marín Rodríguez
<rmrodriguez@carto.com> wrote:

I've written a small patch to add support for pow() in pgbench.

Cool.

The main reason behind it is that I'm currently using a shell call to do it
which takes between 2-10 ms that can be a big percentage of the time taken
by the whole transaction. For example (shortened):

latency average = 11.718 ms
- statement latencies in milliseconds:
2.834 \setshell POWER2 awk 'BEGIN {p=2^ARGV[1]; print p }'
:ZOOM_CURRENT
8.846 SELECT
ST_AsBinary(ST_Simplify(ST_SnapToGrid("the_geom_webmercator",:SNAP),
:SIMPLIFY)) AS geom FROM

I've also updated the related docs and added some tests. Please let me know
if I'm missing anything.

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier wrote:

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.

I think Ra�l would do well to review this patch by Fabien
/messages/by-id/alpine.DEB.2.20.1710201835390.15170@lancre
which adds a few functions and operators.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

Michael Paquier wrote:

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.

I think Raúl would do well to review this patch by Fabien
/messages/by-id/alpine.DEB.2.20.1710201835390.15170@lancre
which adds a few functions and operators.

Good idea. pow() is not added by Fabien's patch, but an operator for
pow() could be something to add as well.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

both patches seem complementary. I've rebased my changes on top of that
patch
(v14) in https://git.io/vFtnT and everything seems to be working fine.

On Mon, Oct 30, 2017 at 12:36 PM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

Michael Paquier wrote:

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.

I think Raúl would do well to review this patch by Fabien
/messages/by-id/alpine.DEB.2.20.

1710201835390.15170@lancre

which adds a few functions and operators.

Good idea. pow() is not added by Fabien's patch, but an operator for
pow() could be something to add as well.
--
Michael

--

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

On Mon, Oct 30, 2017 at 11:56 AM, Raúl Marín Rodríguez
<rmrodriguez@carto.com> wrote:

both patches seem complementary. I've rebased my changes on top of that
patch
(v14) in https://git.io/vFtnT and everything seems to be working fine.

Attaching patches directly to a thread is a better practice as if
github goes away, any Postgres developers can still have an access to
any code you publish using the public archives on postgresql.org.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier wrote:

Attaching patches directly to a thread is a better practice as if
github goes away, any Postgres developers can still have an access to
any code you publish using the public archives on postgresql.org.

Also, by posting to pgsql-hackers indicating intention to integrate to
Postgres you're implicitly making a statement about the license of your
contribution; see the second half of the last paragraph at
https://wiki.postgresql.org/wiki/Archives_Policy

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry about the patch. Attaching it now so it can be considered as
submitted.

--

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

Hello Michaël,

I'm fine with having pow in pgbench.

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.

It might be even better if https://commitfest.postgresql.org/15/985/,
which has been around for over one year (!), get through some day...

--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Raúl,

Sorry about the patch. Attaching it now so it can be considered as
submitted.

There is a typo in the XML doc:

<literal>1024.0/<literal>

Please check that the documentation compiles.

I'm at odds with having the integer version rely on a double pow(), even
if it works. I think that there should be a specific integer version which
does use integer operations. From stack overflow, the following is
suggested:

int ipow(int base, int exp)
{
int result = 1;
while (exp)
{
if (exp & 1)
result *= base;
exp >>= 1;
base *= base;
}

return result;
}

The integer version should be when x & y are integers *AND* y >= 0.

if y is a negative integer, the double version should be used.

--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Fabien,

Thanks for the review.
I've fixed the documentation and added an ipow function that handles both
positive and negative ints, having 0^0 == 1 and 0^(negative) == PG_INT64_MAX
since that's what my glibc math.h pow() is returning.

On Sat, Nov 4, 2017 at 12:34 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello Raúl,

Sorry about the patch. Attaching it now so it can be considered as

submitted.

There is a typo in the XML doc:

<literal>1024.0/<literal>

Please check that the documentation compiles.

I'm at odds with having the integer version rely on a double pow(), even
if it works. I think that there should be a specific integer version which
does use integer operations. From stack overflow, the following is
suggested:

int ipow(int base, int exp)
{
int result = 1;
while (exp)
{
if (exp & 1)
result *= base;
exp >>= 1;
base *= base;
}

return result;
}

The integer version should be when x & y are integers *AND* y >= 0.

if y is a negative integer, the double version should be used.

--
Fabien.

--

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

Hello Raúl,

I've fixed the documentation and added an ipow function that handles both
positive and negative ints, having 0^0 == 1 and 0^(negative) == PG_INT64_MAX
since that's what my glibc math.h pow() is returning.

From the comment:

* For exp < 0 return 0 except when the base is 1 or -1

I think that it should do what POW does in psql, i.e.:

fabien=# SELECT POW(2, -2); # 0.25

that is if exp < 0 the double version should be used, it should
not return 0.

Basically the idea is that the pgbench client-side version should behave
the same as the SQL version.

--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Fabien,

Sorry for the confusion, I wasn't aware that SQL pow changed types
depending on
the input value.

I've modified the function to match more closely the behaviour of SQL,
except
that 0^(negative) returns 'double inf'. Do you think there is any value in
raising an error instead?

On Mon, Nov 6, 2017 at 2:12 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello Raúl,

I've fixed the documentation and added an ipow function that handles both

positive and negative ints, having 0^0 == 1 and 0^(negative) ==
PG_INT64_MAX
since that's what my glibc math.h pow() is returning.

From the comment:

* For exp < 0 return 0 except when the base is 1 or -1

I think that it should do what POW does in psql, i.e.:

fabien=# SELECT POW(2, -2); # 0.25

that is if exp < 0 the double version should be used, it should
not return 0.

Basically the idea is that the pgbench client-side version should behave
the same as the SQL version.

--
Fabien.

--

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

Hello,

Sorry for the confusion, I wasn't aware that SQL pow changed types
depending on the input value.

Indeed, this is quite strange...

fabien=# SELECT i, POW(2, i) FROM generate_series(-2, 2) AS i;
-2 | 0.25
-1 | 0.5
0 | 1
1 | 2
2 | 4

I've modified the function to match more closely the behaviour of SQL,
except that 0^(negative) returns 'double inf'. Do you think there is any
value in raising an error instead?

fabien=# SELECT POW(0,-1);
ERROR: zero raised to a negative power is undefined

Hmmmm... I'm fine with double inf, because exception in pgbench means the
end of the script, which is not desirable for benchmarking purposes.

I think that:

- you can simplify the ipow function by removing handling of y<0 case,
maybe add an assert to be sure to avoid it.

- you should add more symmetry and simplify the evaluation:

if (int & int)
{
i1, i2 = ...;
if (i2 >= 0)
setIntValue(retval, ipow(i1, i2));
else
// conversion is done by C, no need to coerce again
setDoubleValue(retval, pow(i1, i2));
}
else
{
d1, d2 = ...;
setDoubleValue(retval, pow(d1, d2));
}

Add a test case to show what happens on NULL arguments, hopefully the
result is NULL.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

Indeed, this is quite strange...

I don't want to go too deep into it, but you get stuff like this:

Select pow(2.0, -3)::text = pow(2, -3)::text;
?column?
----------
f
(1 row)

- you can simplify the ipow function by removing handling of y<0 case,

maybe add an assert to be sure to avoid it.

I agree, done.

- you should add more symmetry and simplify the evaluation:

Done too.

Add a test case to show what happens on NULL arguments, hopefully the

result is NULL.

Done and it does.

Thanks again for the review.

On Mon, Nov 6, 2017 at 4:14 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Hello,

Sorry for the confusion, I wasn't aware that SQL pow changed types

depending on the input value.

Indeed, this is quite strange...

fabien=# SELECT i, POW(2, i) FROM generate_series(-2, 2) AS i;
-2 | 0.25
-1 | 0.5
0 | 1
1 | 2
2 | 4

I've modified the function to match more closely the behaviour of SQL,

except that 0^(negative) returns 'double inf'. Do you think there is any
value in raising an error instead?

fabien=# SELECT POW(0,-1);
ERROR: zero raised to a negative power is undefined

Hmmmm... I'm fine with double inf, because exception in pgbench means the
end of the script, which is not desirable for benchmarking purposes.

I think that:

- you can simplify the ipow function by removing handling of y<0 case,
maybe add an assert to be sure to avoid it.

- you should add more symmetry and simplify the evaluation:

if (int & int)
{
i1, i2 = ...;
if (i2 >= 0)
setIntValue(retval, ipow(i1, i2));
else
// conversion is done by C, no need to coerce again
setDoubleValue(retval, pow(i1, i2));
}
else
{
d1, d2 = ...;
setDoubleValue(retval, pow(d1, d2));
}

Add a test case to show what happens on NULL arguments, hopefully the
result is NULL.

--
Fabien.

--

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

I don't want to go too deep into it, but you get stuff like this:

Select pow(2.0, -3)::text = pow(2, -3)::text;

Sure. It does so with any overloaded operator or function:

fabien=# SELECT (2.0 + 3)::TEXT = (2 + 3)::TEXT; # f

Patch applies, make check ok in pgbench, doc gen ok.

ipow code is nice and simple.

I switched the patch to "Ready for Committer"

Let's now hope that a committer gets around to consider these patch some
day.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO <coelho@cri.ensmp.fr> 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

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 <michael.paquier@gmail.com>
wrote:

On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO <coelho@cri.ensmp.fr> 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

Hello Raúl,

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

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.

Patch applies cleanly, make check ok.

Back to "ready".

--
Fabien.

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