Re: extend pgbench expressions with functions

On Wed, Dec 16, 2015 at 3:07 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>> On Wed, Dec 16, 2015 at 6:10 AM, Robert Haas <robertmhaas@gmail.com>
>> wrote:
>>>
>>> It looks fine to me except that I think we should spell out "param" as
>>> "parameter" throughout, instead of abbreviating.
>>
>>
>> Fine for me. I have updated the first patch as attached (still looking
>> at the second).
>
>
> This doc update threshold -> parameter & sgml and C code looks ok to me.

So I am having a look at the second patch...

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

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

+      <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? Both
concepts are linked, and for now as I can see the allocation of a \set
variable is actually always an integer.

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?

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

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.

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.

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.
Regards,
-- 
Michael