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

On Wed, Jan 13, 2016 at 10:28 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> Hello Michaël,
>
>>> ISTM that if pgbench is to be stopped, the simplest option is just to
>>> abort
>>> with a nicer error message from the get*Rand function, there is no need
>>> to
>>> change the function signature and transfer the error management upwards.
>>
>>
>> That's fine to me, as long as the solution is elegant.
>
>
> Hmmm, this is subjective:-)

And dependent on personal opinions and views.

> I've decided to stay with the current behavior (\setrandom), that is to
> abort the current transaction on errors but not to abort pgbench itself. The
> check is done before calling the functions, and I let an assert in the
> functions just to be sure. It is going to loop on errors anyway, but this is
> what it already does anyway.

OK, yes I see now I missed that during my last review. This has the
disadvantage to double the amount of code dedicated to parameter
checks though :(
But based on the feedback perhaps other folks would feel that it would
be actually worth simply dropping the existing \setrandom command. I
won't object to that personally, such pgbench features are something
for hackers and devs mainly, so I guess that we could survive without
a deprecation period here.

>>> Ok. I'm hesitating about removing the operator management, especially if
>>> I'm
>>> told to put it back afterwards.
>>
>>
>> I can understand that, things like that happen all the time here and
>> that's not a straight-forward patch that we have here. I am sure that
>> additional opinions here would be good to have before taking one
>> decision or another. With the current statu-quo, let's just do what
>> you think is best.
>
>
> I let the operators alone and just adds functions management next to it.
> I'll merge operators as functions only if it is a blocker.

I think that's a blocker, but I am not the only one here and not a committer.

-                 fprintf(stderr, "gaussian parameter must be at least
%f (not \"%s\")\n", MIN_GAUSSIAN_PARAM, argv[5]);
+                fprintf(stderr,
+                           "random gaussian parameter must be greater than %f "
+                            "(got %f)\n", MIN_GAUSSIAN_PARAM, parameter);
This looks like useless noise to me. Why changing those messages?

-     if (parameter <= 0.0)
+    if (parameter < 0.0)     {

This bit is false, and triggers an assertion failure when the
exponential parameter is 0.
             fprintf(stderr,
-                       "exponential parameter must be greater than
zero (not \"%s\")\n",
-                        argv[5]);
+                      "random exponential parameter must be greater than 0.0 "
+                       "(got %f)\n", parameter);             st->ecnt++;
-            return true;
+           return false;
This diff is noise as well, and should be removed.

+               /*
+                * Note: this section could be removed, as the same
functionnality
+                * is available through \set xxx random_gaussian(...)
+                */
I think that you are right to do that. That's no fun to break existing
scripts, even if people doing that with pgbench are surely experienced
hackers.
                       }
-               case ENODE_VARIABLE:
Such diffs are noise as well.

int() should be strengthened regarding bounds. For example:
\set cid debug(int(int(9223372036854775807) +
double(9223372036854775807)))debug(script=0,command=1): int -9223372036854775808
--
Michael