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

Hello Kyotaro-san,

> 1.evalInt and evalDouble are mutually calling on single expr
> node.

Indeed.

> It looks simple and seems to work but could easily broken
> to fall into infinite call and finally (in a moment) exceeds
> stack depth.

The recursion is on the tree-structure of the expression, which is 
evaluated depth-first. As the tree must be finite, and there is no such 
thing as recursive functions in the expression tree, this should never 
happen. If it happened, it would be a bug in the eval functions, probably 
a forgotten "case", and could be fixed there easily.

> I think it is better not to do so. For example, reunioning
> evalDouble and evalInt into one evalulateExpr() which can return
> both double and int result would do so. The interface would be
> something like the follwings or others. Some individual
> functions might be better to be split out.
>
>  static ExprRetType evaluateExpr(TState,CState,PgBenchExpr,
>                                  int *iret, double *dret);

That would not work as simply as that: this signature does not say whether 
a double or an int is expected, and without this information you would not 
know what to do on some operators (the typing is explicit and descendant 
in the current implementation).

Even if you add this information, or you use the returned type information 
to do the typing dynamically, that would mean testing the result types and 
implementing the necessary casts depending on this information for every 
function within the generic eval, which would result in repetitive and 
ugly code for every function and operator: for instance for '+', you would 
have to implement 4 cases explicitely, i+i, f+i, i+f, f+f as "int add", 
"cast left and double add", "cast right and double add", and finally 
"double add". Then you have other operators to consider... Although some 
sharing can be done, this would end in lengthy & ugly code.

A possible alternative could be to explicitely and statically type all 
operations, adding the necessary casts explicitely, distinguishing 
operators, but the would mean more non-obvious code to "compile" the 
parsed expressions, I'm not sure that pgbench deserves this.

The two eval functions and the descendant typing allow to hide/ignore 
these issues because each eval function handles just one type, type 
boundaries are explicitely handled by calling the other function, so there 
is no reason to test and cast everything all the time.

So I thing that it would not be an improvement to try to go the way you 
suggest.

> 2. You combined abs/sqrt/ddebug and then make a branch for
>  them. It'd be better to split them even if eval*() looks to be
>  duplicate.

Hm, why not.

Here is a v10 where I did that when it did not add too much code 
repetition (eg I kept the shared branches for MIN & MAX and for the 3 
RANDOM functions so as to avoid large copy pastes).

-- 
Fabien.