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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
