|From:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|To:||Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|Cc:||PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: extend pgbench expressions with functions|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
> + <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.
> + /* beware that the list is reverse in make_func */
> #ifdef DEBUG
> Some noise.
> 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
> You want to emulate with complex numbers instead?
> 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.
|Next Message||Steve Singer||2016-01-16 19:55:50||Re: pglogical - logical replication contrib module|
|Previous Message||Andres Freund||2016-01-16 17:47:53||Re: WIP: Rework access method interface|