On Thu, Jan 7, 2016 at 7:00 PM, Fabien COELHO wrote:
> Obviously all this is possible in the grand scale of things, but this is
> also significant work for a tiny benefit, if any. I rather see "debug" as a
> simple tool for debugging a script with "pgbench -t 1" (i.e. one or few
> transactions), so that the trace length is not an issue.
> Once the script is satisfactory, remove the debug and run it for real.
> Note that it does not make much sense to run with "debug" calls for many
> transactions because of the performance impact.
Yep. That's exactly what I did during my tests.
>> And more comments for example in pgbench.h would be welcome to explain
>> more the code.
>
> Ok. I'm generally in favor of comments.
OK, I added some where I thought it was adapted.
>> I am fine to take a final look at that before handling it to a committer
>> though. Does that sound fine as a plan, Fabien?
>
> I understand that you propose to add these comments?
Yep. I did as attached.
> I'm fine with that!:-)
> If I misuderstood, tell me:-)
I think you don't, but I found other issues:
1) When precising a negative parameter in the gaussian and exponential
functions an assertion is triggered:
Assertion failed: (parameter > 0.0), function getExponentialRand, file
pgbench.c, line 494.
Abort trap: 6 (core dumped)
An explicit error is better.
2) When precising a value between 0 and 2, pgbench will loop
infinitely. For example:
\set cid debug(random_gaussian(-100, 100, 0))
In both cases we just lack sanity checks with PGBENCH_RANDOM,
PGBENCH_RANDOM_EXPONENTIAL and PGBENCH_RANDOM_GAUSSIAN. I think that
those checks would be better if moved into getExponentialRand & co
with the assertions removed. I would personally have those functions
return a boolean status and have the result in a pointer as a function
argument.
Another thing itching me is that ENODE_OPERATOR is actually useless
now that there is a proper function layer. Removing it and replacing
it with a set of functions would be more adapted and make the code
simpler, at the cost of more functions and changing the parser so as
an operator found is transformed into a function expression.
I am attaching a patch where I tweaked a bit the docs and added some
comments for clarity. Patch is switched to "waiting on author" for the
time being.
Regards,
--
Michael