From: | ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | "Brendan Jurd" <direvus(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Alvaro Herrera" <alvherre(at)commandprompt(dot)com>, "Greg Smith" <gsmith(at)gregsmith(dot)com> |
Subject: | Re: [Review] pgbench duration option |
Date: | 2008-09-05 05:47:23 |
Message-ID: | 20080905142210.7A24.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
"Brendan Jurd" <direvus(at)gmail(dot)com> wrote:
> Josh assigned your patch to me for an initial review. Here's what I
> have so far.
Thank your for reviewing!
> 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.
Good news.
> 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 found I have a mistake in the usage of BOOL and BOOLEAN types.
I'll fix it in the next patch.
> 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".
"invalid duration (-T): %d" looks good for me.
> * 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.
Ok, I'll add the error protection.
> * It seems like the code to stop pgbench when the timer has expired
> has some unnecessary duplication.
Ah, I forgot to clean up the codes. I'll fix it. (There was another
logic here in the first patch, but not needed in the version.)
> * 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. Use the -t or -T option to make the run last at least a few
> minutes, so as to average out noise.
I'll do it.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Volkan YAZICI | 2008-09-05 06:15:19 | Re: Verbosity of Function Return Type Checks |
Previous Message | Brendan Jurd | 2008-09-05 05:07:00 | [Review] pgbench duration option |