Re: pow support for pgbench

Hi,

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

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.

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

On Fri, Oct 27, 2017 at 4:51 PM, Raúl Marín Rodríguez
<rmrodriguez(at)carto(dot)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

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
https://www.postgresql.org/message-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

On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
<alvherre(at)alvh(dot)no-ip(dot)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
> https://www.postgresql.org/message-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

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(dot)paquier(at)gmail(dot)com
> wrote:

> On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
> <alvherre(at)alvh(dot)no-ip(dot)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
> > https://www.postgresql.org/message-id/alpine.DEB.2.20.
> 1710201835390(dot)15170(at)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(at)carto(dot)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

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

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.

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.

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(at)cri(dot)ensmp(dot)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.

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(at)cri(dot)ensmp(dot)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.

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(at)cri(dot)ensmp(dot)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.

On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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(dot)paquier(at)gmail(dot)com>
wrote:

> On Tue, Nov 7, 2017 at 1:34 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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

On Fri, Dec 1, 2017 at 4:57 AM, Raúl Marín Rodríguez
<rmrodriguez(at)carto(dot)com> 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

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.

>> 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.

Hi,

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

Fixed in the attached patch.

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?

On Fri, Dec 1, 2017 at 8:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Dec 1, 2017 at 4:57 AM, Raúl Marín Rodríguez
> <rmrodriguez(at)carto(dot)com> 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
>

--

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

>> 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.

On Mon, Dec 4, 2017 at 10:47 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> 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

On Tue, Dec 5, 2017 at 5:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> 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

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Tue, Dec 5, 2017 at 5:38 AM, Robert Haas <robertmhaas(at)gmail(dot)com> 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

> 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.

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.

regards, tom lane

On Tue, Dec 5, 2017 at 11:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

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.

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?

> 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.

On Tue, Dec 5, 2017 at 7:44 AM, Raúl Marín Rodríguez
<rmrodriguez(at)carto(dot)com> 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

>> 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.

Raúl Marín Rodríguez <rmrodriguez(at)carto(dot)com> wrote:

> 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

Indeed, to me, that has turned out to be the most intriguing part of
the whole thread. Needs to be in some SQL subtleties exam somewhere:

select pow(2.0,-3), pow(2,-3);
pow | pow
--------------------+-------
0.1250000000000000 | 0.125

Looks like the first call resolves to the numeric version, while
the second (with integer arguments) resolves to the double one:

select pow(2.0,-3) is of (numeric), pow(2,-3) is of (double precision);
?column? | ?column?
----------+----------
t | t

Still, a numeric 0.125 doesn't always have those trailing zeros:

select pow(2.0,-3), pow(2,-3)::numeric;
pow | pow
--------------------+-------
0.1250000000000000 | 0.125

What's going on in the representation?

select numeric_send(pow(2.0,-3)), numeric_send(pow(2,-3)::numeric);
numeric_send | numeric_send
------------------------+------------------------
\x0001ffff0000001004e2 | \x0001ffff0000000304e2

I assume the 10 vs. 03 hex in the tails of those things represent
either 'precision' or 'scale' of 16 vs. 3? I don't get much help
from IS OF (numeric(p,s)), which seems to ignore any p,s and just
be true for any numeric. But here, this matches:

select numeric_send(0.125::numeric(16,16));
numeric_send
------------------------
\x0001ffff0000001004e2

How does numeric_power choose the precision and scale of its result?
Is that something the standard dictates?

Given that 0.125 is exact for this answer, at first I wanted to
ask if numeric_power could be made to produce the result with
precision 3, but then I realized that's backwards. A result with
precision 3 would be like saying, eh, it's somewhere between
0.1245 and 0.1255. If a result is known to be exact, it would be
better to go the other way and return it as numeric(huge).

That then led me to wonder if the cast float8_numeric is really
doing the right thing. Is it turning 0.125 (an exact representation
as float8) into numeric(3,3), again hedging as if it might be anything
from 0.1245 to 0.1255? Would it be better for float8_numeric to
produce a numeric with the precision/scale reflecting the actual
limits of float8?

Ok, now I've been driven to UTSL. It looks as if the intent of
the snprintf(..., "%.*g", DBL_DIG, val) in float8_numeric could
have been to accomplish that. It doesn't, though, as (at least
on my platform), %g drops trailing zeros, though there
is a documented 'alternate form' flag # that prevents that.
It works in bash:

bash-4.2$ printf '%.*g\n' 15 0.125
0.125
bash-4.2$ printf '%#.*g\n' 15 0.125
0.125000000000000

Does the standard prescribe how cast(float8 as numeric) ought to
select the precision/scale?

Sorry to drift OT, as this is more about the SQL functions than
pgbench, but it was too puzzling to ignore. :)

-Chap

> 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.

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 <coelho(at)cri(dot)ensmp(dot)fr> 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

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.

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 <coelho(at)cri(dot)ensmp(dot)fr> 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

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.

On Fri, Dec 22, 2017 at 12:46 AM, Raúl Marín Rodríguez
<rmrodriguez(at)carto(dot)com> 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

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.

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

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

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.

On Tue, Dec 26, 2017 at 4:45 PM, Raúl Marín Rodríguez
<rmrodriguez(at)carto(dot)com> 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