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

On Fri, Oct 30, 2015 at 1:01 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> Here is a v12 which implements the suggestions below.

Sorry it's taken me so long to get around to looking at this.

Some thoughts on an initial read-through:

1. I think there should really be two patches here, the first adding
functions, and the second adding doubles.  Those seem like separate
changes.  And offhand, the double stuff looks a lot less useful that
the function call syntax.

2. ddebug and idebug seem like a lousy idea to me.  If we really need
to be able to print stuff from pgbench, which I kind of doubt, then
maybe we should have a string data type and a print() function, or
maybe a string data type and a toplevel \echo command.

3. I'm perplexed by why you've replaced evaluateExpr() with evalInt()
and evalDouble().  That doesn't seem similar to what I've seen in
other expression-evaluation engines.  Perhaps I could find out by
reading the comments, but actually not, because this entire patch
seems to add only one comment:

+       /* reset column count for this scan */
+       yycol = 0;

While I'm not a fan of excessive commenting, I think a little more
explanation here would be good.

4. The table of functions in pgbench.sgml seems to leave something to
be desired.  We added a pretty detailed write-up on the Gaussian and
exponential options to \setrandom, but exporand() has only this
description:

+      <row>
+       <entry><literal><function>exporand(<replaceable>i</>,
<replaceable>j</>, <replaceable>t</>)</></></>
+       <entry>integer</>
+       <entry>exponentially distributed random integer in the bounds,
see below</>
+       <entry><literal>exporand(1, 10, 3.0)</></>
+       <entry>int between <literal>1</> and <literal>10</></>
+      </row>

That's not very helpful.  Without looking at the example, there's no
way to guess what i and j mean, and even with looking at the example,
there's no way to guess what t means.  If, as I'm guessing, exporand()
and guassrand() behave like \setrandom with the exponential and/or
Gaussian options, then the documentation for one of those things
should contain all of the detailed information and the documentation
for the other should refer to it.  More than likely, exporand() and
gaussrand() should get the detailed explanation, and \setrandom should
be document as a deprecated alternative to \set ...
{gauss,expo,}rand(...)

5. I liked Heikki's proposed function names random_gaussian(min, max,
threshold) and random_exponential(min, max, threshold) better than the
ones you've picked here.  I think random() would be OK instead of his
suggestion of random_uniform(), though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company