Re: extend pgbench expressions with functions

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: extend pgbench expressions with functions
Date: 2015-09-15 17:26:51
Message-ID: alpine.DEB.2.10.1509151844080.6503@sto
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2015-09-15 17:30:10 Re: extend pgbench expressions with functions
Previous Message YUriy Zhuravlev 2015-09-15 17:16:10 Re: Move PinBuffer and UnpinBuffer to atomics