Re: extend pgbench expressions with functions - Mailing list pgsql-hackers

Hello Michael,

Thanks for your remarks.

> +      double constants such as <literal>3.14156</>,
> You meant perhaps 3.14159 :)

Indeed!

> +       <entry><literal><function>max(<replaceable>i</>,
> <replaceable>...</>)</></></>
> +       <entry>integer</>
> Such function declarations are better with optional arguments listed
> within brackets.

Why not. I did it that way because this is the standard C syntax for 
varargs.

> +      <row>
> +       <entry><literal><function>double(<replaceable>i</>)</></></>
> +       <entry>double</>
> +       <entry>cast to double</>
> +       <entry><literal>double(5432)</></>
> +       <entry><literal>5432.0</></>
> +      </row>
> +      <row>
> +       <entry><literal><function>int(<replaceable>x</>)</></></>
> +       <entry>integer</>
> +       <entry>cast to int</>
> +       <entry><literal>int(5.4 + 3.8)</></>
> +       <entry><literal>9</></>
> +      </row>
> If there are cast functions, why doesn't this patch introduces the
> concept of the data type for a variable defined in a script?

Because this would be a pain and this is really useless as far as pgbench 
scripts are concerned, it really just needs integers.

> Both concepts are linked, and for now as I can see the allocation of a 
> \set variable is actually always an integer.

Yes.

> In consequence, sqrt behavior is a bit surprising, for example this script:
> \set aid sqrt(2.0)
> SELECT :aid;
> returns that:
> SELECT 1;
> Shouldn't a user expect 1.414 instead? Fabien, am I missing a trick?

There is no trick. There are only integer variables in pgbench, so 1.4 is 
rounded to 1 by the assignment.

> It seems to me that this patch would gain in clarity by focusing a bit
> more on the infrastructure first and remove a couple of things that
> are not that mandatory for now...

This is more or less what I did in the beginning, and then someone said 
"please show a complete infra with various examples to show that the infra 
is really extensible". Now you say the reverse. This is *tiring* and is 
not a good use of the little time I can give.

> So the following things are not necessary as of now:

> - cast functions from/to int/double, because a result variable of a
> \set does not include the idea that a result variable can be something
> else than an integer. At least no options is given to the user to be
> able to make a direct use of a double value.
> - functions that return a double number: sqrt, pi
> - min and max have value because they allow a user to specify the
> expression once as a variable instead of writing it perhaps multiple
> times in SQL, still is it enough to justify having them as a set of
> functions available by default? I am not really sure either.

AFAICR this is because I was *ASKED* to show an infra which would deal 
with various cases: varargs, double/int functions/operators, overloading, 
so I added some example in each category, hence the current list of 
functions in the patch.

> Really, I like this patch, and I think that it is great to be able to
> use a set of functions and methods within a pgbench script because
> this clearly can bring more clarity for a developer, still it seems
> that this patch is trying to do too many things at the same time:
> 1) Add infrastructure to support function calls and refactor the
> existing functions to use it. This is the FUNCTION stuff in the
> expression scanner.
> 2) Add the concept of double return type, this could be an extension
> of \set with a data type, or a new command like \setdouble. This
> should include as well the casting routines I guess. This is the
> DOUBLE stuff in the expression scanner.
> 3) Introduce new functions, as needed.

Yep. I started with (1), and was *ASKED* to do the others.

Adding double variables in pretty useless in my opinion, and potentially 
lead to issues in various places, so I'm not in a hurry to do that.

> 1) and 2) seem worthwhile to me. 3) may depend on the use cases we'd
> like to have.. sqrt has for example value if a variable can be set
> double as a double type.

Sure. This is just an example of a double function so that if someone 
wants to add another one they can just update where it make sense. Maybe 
log/exp would make more sense for pgbench.

> In conclusion, for this CF the patch doing the documentation fixes is
> "Ready for committer", the second patch introducing the function
> infrastructure should focus more on its core structure and should be
> marked as "Returned with feedback". Opinions are welcome.

My opinion is that to do and unddo work because of random thoughts on the 
list is tiring.

What I can do is:

(1) fix the doc and bugs if any, obviously.

(2a) remove double stuff, just keep integer functions.     I would rather keep min/max, though.

(2b) keep the patch with both int & double as is.

What I will *NOT* do is to add double variables without a convincing use 
case.

-- 
Fabien.