On Tue, Aug 19, 2008 at 12:24 PM, ITAGAKI Takahiro
<itagaki.takahiro@oss.ntt.co.jp> wrote:
> Ok, I rewrote the patch to use SIGALRM instead of gettimeofday.
>
Hi Itagaki-san,
Josh assigned your patch to me for an initial review. Here's what I
have so far.
The patch applies cleanly on the latest git HEAD, and
compiles/installs/runs nicely with a fresh initdb on amd64 gentoo.
I didn't notice any aberrations in the coding style.
The -T option seems to work as advertised, and I wasn't able to detect
any performance degradation (or a significant variation of any kind)
using the -T option versus the -t option.
Unfortunately, I don't have access to a Windows build environment, so
I wasn't able to investigate the portability concerns raised in the
email thread.
I do have a few minor suggestions:
* The error message given for -T <= 0 is "invalid number of
duration(-T): %d". The grammar isn't quite right there. I would go
with "invalid run duration (-T): %d", or perhaps just "invalid
duration (-T): %d".
* If the -T and -t options are supposed to be mutually incompatible,
then there should be an error if the user tries to specify both
options. Currently, if I specify both options, the -t option is
ignored in favour of -T. An error along the lines of "Specify either
a number of transactions (-t) or a duration (-T), not both." would be
nice.
* It seems like the code to stop pgbench when the timer has expired
has some unnecessary duplication. Currently it reads like this:
if (duration > 0) { if (timer_exceeded) { <stop> }
} else if (st->cnt >= nxacts) { <stop> }
Wouldn't this be better written as:
if ((duration > 0 && timer_exceeded) || st->cnt >= nxacts) { <stop> }
* The documentation should mention the new -T option in the following paragraph:
In the first place, never believe any test that runs for only a few
seconds. Increase the -t setting enough to make the run last at least
a few minutes, so as to average out noise.
Perhaps:
In the first place, never believe any test that runs for only a few
seconds. Use the -t or -T option to make the run last at least a few
minutes, so as to average out noise.
That's it for my initial review. I hope my comments are helpful.
Cheers,
BJ
