Skip site navigation (1) Skip section navigation (2)

[Review] pgbench duration option

From: "Brendan Jurd" <direvus(at)gmail(dot)com>
To: "ITAGAKI Takahiro" <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
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: [Review] pgbench duration option
Date: 2008-09-05 05:07:00
Message-ID: 37ed240d0809042207q60c01beew36f5a4b1975f6814@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Tue, Aug 19, 2008 at 12:24 PM, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)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

In response to

Responses

pgsql-hackers by date

Next:From: ITAGAKI TakahiroDate: 2008-09-05 05:47:23
Subject: Re: [Review] pgbench duration option
Previous:From: David E. WheelerDate: 2008-09-05 04:42:20
Subject: Re: [Review] Tests citext casts by David Wheeler.

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group