Re: extend pgbench expressions with functions

On Mon, Feb 15, 2016 at 5:21 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Feb 14, 2016 at 11:28 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> Here is a 3 part v29:
>>
>> a) add support for integer functions in pgbench, including turning
>>    operators as functions, as well as some minimal infrastructure for
>>    additional types (this allows to minimize the diffs with the next
>>    patch which adds double).
>>
>> b) add support for doubles, including setting double variables.
>>    Note that variable are not explicitely typed. Add some
>>    double-related functions, most interesting of them for pgbench
>>    are the randoms.
>>
>> c) remove \setrandom support (as thanks to part b \set x random(...) does
>>    the same). Prints an error pointing to the replacement if \setrandom is
>>    used in a pgbench script.
>
> Thanks, I'll review these as soon as I can get to it.

I got around to look at (a) in this set.

+   if ((PGBENCH_FUNCTIONS[fnumber].nargs >= 0 &&
+        PGBENCH_FUNCTIONS[fnumber].nargs != elist_length(args)) ||
+       /* check at least one arg for min & max */
+       (PGBENCH_FUNCTIONS[fnumber].nargs == -1 &&
+        elist_length(args) == 0))
+       expr_yyerror_more("unexpected number of arguments",
+                         PGBENCH_FUNCTIONS[fnumber].fname);
We could split that into two parts, each one with a more precise error message:
- For functions that expect at least one argument: "at least one
argument was expected, none found".
- For functions that expect N arguments: "N arguments expected, but M found"

+       "\\set aid random(1, " CppAsString2(naccounts) " * :scale)\n"       "SELECT abalance FROM pgbench_accounts
WHEREaid = :aid;\n"   }};
 

-/* Function prototypes */
Noise.

+ * Recursive evaluation of int or double expressions
+ *
+ * Note that currently only integer variables are available, with values
+ * stored as text.
This comment is incorrect, we only care about integers in this patch.

Taking patch 1 as a completely independent thing, there is no need to
introduce PgBenchValueType yet. Similar remark for setIntValue and
coerceToInt. They are useful afterwards when introducing double types
to be able to handle double input parameters for the gaussian and
other functions.

-                       /*
-                        * INT64_MIN / -1 is problematic, since the result
-                        * can't be represented on a two's-complement machine.
-                        * Some machines produce INT64_MIN, some produce zero,
-                        * some throw an exception. We can dodge the problem
-                        * by recognizing that division by -1 is the same as
-                        * negation.
-                        */
-                       if (rval == -1)
+                       if (coerceToInt(&rval) == -1)                       {
-                           *retval = -lval;
-
-                           /* overflow check (needed for INT64_MIN) */
-                           if (lval == PG_INT64_MIN)
-                           {
-                               fprintf(stderr, "bigint out of range\n");
-                               return false;
-                           }
+                           setIntValue(retval, 0);
+                           return true;                       }
(INT64_MIN / -1) should error. (INT64_MIN % -1) should result in 0.
This is missing the division handling.
-- 
Michael