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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
