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

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.