Hello Alvaro,
> On top of evaluateExpr() we need a comment (generally I think pgbench
> could do with more comments; not saying your patch should add them, just
> expressing an opinion.)
Having spent some time in pgbench, I agree that more comments are a good
thing.
> Also, intuitively I would say that the return values of that function
> should be reversed: return true if things are good.
Ok.
> Can we cause a stack overflow in this function with a complex
> enough expression?
Not for any practical purpose, IMO.
> I wonder about LOCATE and LOCATION. Can we do away with the latter, and
> keep only LOCATE perhaps with a better name such as PRINT_ERROR_AT or
> similar? I would just expand an ad-hoc fprintf in the single place
> where the other macro is used.
I think that all "location" information should always be the same, so
having it defined only once helps maintenance. If someone fixes the macro
and there is one expanded version it is likely that it would not be
changed. Maybe we could do with only one macro, though.
> Are we okay with only integer operands? Is this something we would
> expand in the future?
Probably
> Is the gaussian/exp random stuff going to work with integer operands,
No, it will need a floating point parameter, but I was thinking of only
adding constants floats as function arguments in a first approach, and not
allow an expression syntax on these, something like:
\set n exprand(1, :size+3, 2.0) + 1
But not
\set n exprand(1, :size+3, :size/3.14159) + 1
That is debatable. Otherwise we have to take care of typing issues, which
would complicate the code significantly with two dynamic types (int &
float) to handle, propagate and so in the expression evaluation. It is
possible though, but it seems to me that it is a lot of bother for a small
added value.
Anyway, I suggest to keep that for another round and keep the Robert's
isofunctional patch as it is before extending.
> if we want to change it to use function syntax, as expressed elsewhere?
I think I'll add a function syntax, and add a new node type to handle
these, but the current syntax should/might be preserved for upward
compatibility.
--
Fabien.
