Обсуждение: [HACKERS] 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
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


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


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

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

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

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

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

>> 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@cri.ensmp.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@gmail.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.paquier@gmail.com> writes:
> On Tue, Dec 5, 2017 at 5:38 AM, Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.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.

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

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.

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

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