Re: extend pgbench expressions with functions

Hello Michaël,

>> 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 :(

Yep, I noticed that obviously, but I envision that "\setrandom" pretty
unelegant code could go away soon, so this is really just temporary.

> But based on the feedback perhaps other folks would feel that it would
> be actually worth simply dropping the existing \setrandom command.

Yep, exactly my thoughts. I did not do it because there are two ways:
actually remove it and be done, or build an equivalent \set at parse time,
so that would just remove the execution part, but keep some ugly stuff in
parsing.

I would be in favor of just dropping it.

> 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.

Yep. I can remove it, but I would like a clear go/vote before doing that.

>>> 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.

Ok, I can remove that easily anyway.

> - 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?

Because the message was not saying that it was about random, and I think
that it is the important.

> - if (parameter <= 0.0)
> + if (parameter < 0.0)
> {
>
> This bit is false, and triggers an assertion failure when the
> exponential parameter is 0.

Oops:-( Sorry.

> 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.

Ok, I can but "zero" and "not" back, but same remark as above, why not
tell that it is about random? This information is missing.

> + /*
> + * 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.

Ok, but I would like a clear go or vote before doing this.

> -
> case ENODE_VARIABLE:
> Such diffs are noise as well.

Yep.

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

Hmmm. You mean just to check the double -> int conversion for overflow,
as in:

SELECT (9223372036854775807::INT8 +
9223372036854775807::DOUBLE PRECISION)::INT8;

Ok.

--
Fabien.