On Fri, Sep 18, 2015 at 10:21 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
> > Hello Kyotaro-san, > > My description should have been obscure. Indeed the call tree is >> finite for *sane* expression node. But it makes infinit call for >> a value of expr->etype unknown by both evalDouble and >> evalInt. >> > > Such issue would be detected if the function is actually tested, hopefully > this should be the case... :-) > > However I agree that relying implicitely on the "default" case is not very > good practice, so I updated the code in the attached v11 to fail > explicitely on such errors. > > I also attached a small test script, which exercises most (all?) functions: > > ./pgbench -f functions.sql -t 1 > A short review from me: 1. Patch applies cleanly on current HEAD. 2. It compiles without errors or warnings. 3. The attached test case can be executed w/o symptoms of any problem and it produces meaningful results. Should we not allow for functions taking 0 arguments? Since we're already into some math here, how about pi()? ;-) I understand requiring at least 1 arg simplifies the code a bit, but right now it reports syntax error for "random()", while it correctly reports unexpected number of arguments for "random(1,2,3)". We would need another check for min() and max() which expect >=1 arguments, but it's easy to add. I would also argue that we should rename "random" to "rand" here to avoid confusion with the familiar SQL function "random()" that doesn't take arguments. -- Alex
