Re: extend pgbench expressions with functions

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
Date: 2016-01-16 18:10:05
Message-ID: alpine.DEB.2.10.1601161619480.18181@sto
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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.

Attachment Content-Type Size
pgbench-funcs-20.patch text/x-diff 39.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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