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

Hello Michaël,

> +       <entry>uniformly-distributed random integer in <literal>[lb,ub]</></>

> Nitpick: when defining an interval like that, you may want to add a
> space after the comma.

Why not.

> +      /* beware that the list is reverse in make_func */
> s/reverse/reversed/?

Indeed.

> +
> #ifdef DEBUG
> Some noise.

Ok.

> With this example:
> \set cid debug(sqrt(-1))
> I get that:
> debug(script=0,command=1): double nan
> An error would be more logical, no?

If "sqrt(-1)" as a double is Nan for the computer, I'm fine with that. It 
makes the code simpler to just let the math library do its stuff and not 
bother.

> You want to emulate with complex numbers instead?

Nope.

> The basic operator functions also do not check for integer overflows.

This is a feature. I think that they should not check for overflow, as in 
C, this is just int64_t arithmetic "as is".

Moreover, it would be a new feature to add such a check if desirable, so 
it would belong to another patch, it is not related to adding functions.
The addition already overflows in the current code.

Finally I can think of good reason to use overflows deliberately, so I 
think it would argue against such a change.

> Those three ones are just overflowing:
> \set cid debug(9223372036854775807 + 1)
> \set cid debug(-9223372036854775808 - 1)
> \set cid debug(9223372036854775807 * 9223372036854775807)
> debug(script=0,command=1): int -9223372036854775807
> debug(script=0,command=2): int 9223372036854775807
> debug(script=0,command=3): int 1

All these results are fine from my point of view.

> And this one generates a core dump:
> \set cid debug(-9223372036854775808 / -1)
> Floating point exception: 8 (core dumped)

This one is funny, but it is a fact of int64_t life: you cannot divide 
INT64_MIN by -1 because the result cannot be represented as an int64_t.
This is propably hardcoded in the processor. I do not think it is worth 
doing anything about it for pgbench.

> A more general comment: what about splitting all the execution
> functions into a separate file exprexec.c? evaluateExpr (renamed as
> execExpr) is the root function, but then we have a set of static
> sub-functions for each node, like execExprFunc, execExprVar,
> execExprConst, etc?

I do not see a strong case for renaming. The function part could be split 
because of the indentation, though.

> This way we would save a bit of tab-indentation, this patch making the 
> new code lines becoming larger than 80 characters because of all the 
> switch/case stuff that gets more complicated.

I agree that the code is pretty ugly, but this is partly due to postgres 
indentation rules for switch which are *NOT* reasonnable, IMO.

I put the function evaluation in a function in the attached version.

-- 
Fabien.