Re: extend pgbench expressions with functions

On Fri, Jan 29, 2016 at 5:16 AM, Fabien COELHO <
fabien(dot)coelho(at)mines-paristech(dot)fr> wrote:
> v22 compared to previous:

Thanks for the new patch!

> - remove the short macros (although IMO it is a code degradation)

FWIW, I like this suggestion from Robert.

> - check for INT64_MIN / -1 (although I think it is useless)
> - try not to remove/add blanks lines
> - let some assert "as is"
> - still exit on float to int overflow, see arguments in other mails

+make_op(const char *operator, PgBenchExpr *lexpr, PgBenchExpr *rexpr)
+{
+ return make_func(find_func(operator),
+ /* beware that the list is
reversed in make_func */
+ make_elist(rexpr,
make_elist(lexpr, NULL)));
+}
I think that this should use as argument the function ID, aka PGBENCH_ADD
or similar instead of find_func() with an operator character. This saves a
couple of instructions.

+ * - nargs: number of arguments (-1 is a special value for min & max)
My fault perhaps, it may be better to mention here that -1 means that min
and max need at least one argument, and that the number of arguments is not
fixed.

+ case PGBENCH_MOD:
+ if (coerceToInt(&rval) == 0)
{
fprintf(stderr,
"division by zero\n");
return false;
}
- *retval = lval % rval;
+ if (coerceToInt(&lval) ==
INT64_MIN && coerceToInt(&rval) == -1)
+ {
+ fprintf(stderr,
"cannot divide INT64_MIN by -1\n");
+ return false;
+ }
For mod() there is no need to have an error, returning 0 is fine. You can
actually do it unconditionally when rval == -1.

+ setDoubleValue(retval, d < 0.0? -d: d);
Nitpick: lack of spaces between the question mark.

+typedef enum
+{
+ PGBT_NONE,
+ PGBT_INT,
+ PGBT_DOUBLE
+} PgBenchValueType;
NONE is used nowhere, but I think that you could use it for an assertion
check here:
+ if (retval->type == PGBT_INT)
+ fprintf(stderr, "int " INT64_FORMAT "\n",
retval->u.ival);
+ else if (retval->type == PGBT_DOUBLE)
+ fprintf(stderr, "double %f\n",
retval->u.dval);
+ else
+ fprintf(stderr, "none\n");
Just replace the "none" message by Assert(type != PGBT_NONE) for example.

Another remaining item: should support for \setrandom be dropped? As the
patch is presented this remains intact.
--
Michael