Re: extend pgbench expressions with functions

On Sat, Feb 13, 2016 at 4:37 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>> For example, I just realized that this patch allows values to be
>> either a double or an integer and extends the operators to handle
>> double values.  But variables can still only be integers.
>
> Indeed.

That's exactly the first impression I got about this patch when sending [1].
[1]: http://www.postgresql.org/message-id/CAB7nPqRHVFvnrAHaMarOniedeC90pf3-Wn+U5ccB4reD-HtJnw@mail.gmail.com
By focusing only on integers the patch would largely gain in
simplicity. Now being able to have double values is actually useful
for nested function calls, nothing else.

>> I don't think variables should be explicitly typed but they should be able
>> to store a value of any type that expression evaluation can generate.
>
> Doubles are not really needed that much, it is just to provide something to
> random_* functions parameter, otherwise it is useless as far as pgbench is
> really concerned.

+1.

>> Also, as I said back in November, there's really two completely
>> separate enhancements in here.  One of them is to support a new data
>> type (doubles) and the other is to support functions.
>
> Yep. The first part is precisely the patch I initially submitted 5 CF ago.
> Then I'm asked to put more things in it to show that it can indeed handle
> another type. Then I'm told "you should not have done that". What can I say?

I think that you did your job here by answering the comments of all
the reviewers that had a look at this patch. That's not an easy task.

>> [...] I find implementing operators as functions in disguise not to be one
>> of PostgreSQL's most awesome design decisions, and here we are copying that
>> into pgbench for, well, no benefit that I can see, really.
>
> Well, I did that initially, then I was asked to implements operators as
> functions. It probably saves some lines, so it is not too bad, even if the
> benefit is limited.

Put the blame on me for this one then. I suggested this idea because
Postgres is doing the same, and because it simplifies slightly the
union structure in charge of holding the parsed structures, making the
code a bit more readable IMO.

>> [...] If neither of you are willing to split this patch, I'm not willing
>> to commit it.
>
> If I'm reading you correctly, you would consider committing it:
>
>  - if the function & double stuff are separated ?
>  - for the double part, if variables can be double ?

I just double-checked and could not see a clear use case mentioned in
this thread for double return types, so I would suggest focusing on
the integer portion with min(), max(), abs(), debug() and the existing
functions refactored. That's what your first versions did. If someone
is wishing to implement double types, this someone could do it, the
infrastructure that this patch puts in place has already proved that
it can be easily extended.
-- 
Michael