Re: [Review] pgbench duration option

Hello,

Here is a patch to add duration option (-T) to pgbench instead of
number of transactions (-t). -t and -T are mutually exclusive.

$ pgbench -n -S -c10 -T10
transaction type: SELECT only
scaling factor: 1
query mode: simple
number of clients: 10
number of transactions actually processed: 22522 in 10 s
tps = 2248.686427 (including connections establishing)
tps = 2298.896332 (excluding connections establishing)

I have some experiences where duration-based tests are better than
num-of-xacts-based. For example, testing checkpoints with pgbench
(checkpoints occurs periodically) or endurance test over a weekend
(it should run through 48 hours).

BTW, we have the following notes in the documentation:

| F.18.5. Good Practices
| 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.

If there were -T option, we could rewrite it as
"Increate the -T setting to 300 (5 minutes) or larger" or something.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> Here is a patch to add duration option (-T) to pgbench instead of
> number of transactions (-t). -t and -T are mutually exclusive.

This seems like a fairly bad idea, because it introduces a
gettimeofday() call per transaction. On lots of (admittedly mostly
low-end) hardware, that will impose enough overhead to seriously
affect the results. Worse, the overhead is imposed on the client
side, which is already the bottleneck for pgbench tests.

If this were worth doing (which IMHO it isn't) the appropriate
implementation would be to set up a timer signal handler that would set
a flag to shut down the test after the appropriate amount of time.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > Here is a patch to add duration option (-T) to pgbench instead of
> > number of transactions (-t). -t and -T are mutually exclusive.
>
> This seems like a fairly bad idea, because it introduces a
> gettimeofday() call per transaction. On lots of (admittedly mostly
> low-end) hardware, that will impose enough overhead to seriously
> affect the results.

Are there any evidence about the overhead of gettimeofday here?
I think overhead of libpq API and pgbench itself (including generating
SQLs) are far bigger than gettimeofday.

> set up a timer signal handler that would set
> a flag to shut down the test after the appropriate amount of time.

There would be some porting problems in a timer singal hander.

$ pgbench -n -S -c20 -T10 -M prepared
transaction type: SELECT only
scaling factor: 1
query mode: prepared
number of clients: 20
number of transactions actually processed: 25978 in 10 s
tps = 2589.707165 (including connections establishing)
tps = 2707.804560 (excluding connections establishing)

$ pgbench -n -S -c20 -t1250 -M prepared
transaction type: SELECT only
scaling factor: 1
query mode: prepared
number of clients: 20
number of transactions per client: 1250
number of transactions actually processed: 25000/25000
tps = 2551.834131 (including connections establishing)
tps = 2671.118531 (excluding connections establishing)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Tue, 12 Aug 2008, Tom Lane wrote:

> This seems like a fairly bad idea, because it introduces a
> gettimeofday() call per transaction.

There's already lots of paths through pgbench that introduce gettimeofday
calls all over the place. I fail to see how this is any different.

> If this were worth doing (which IMHO it isn't)

I think that switching the recommended practice for running pgbench to
something time-based rather than transactions-based would increase the
average quality of results people got considerably. How many times do you
see people posting numbers that worthless because the test ran for a
trivial amount of time? Seems like it happens a lot to me. This patch
was already on my todo list for 8.4 and I'm glad I don't have to write it
myself now.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith <gsmith(at)gregsmith(dot)com> writes:
> On Tue, 12 Aug 2008, Tom Lane wrote:
>> This seems like a fairly bad idea, because it introduces a
>> gettimeofday() call per transaction.

> There's already lots of paths through pgbench that introduce gettimeofday
> calls all over the place. I fail to see how this is any different.

You haven't thought about it very hard then. The gettimeofday calls
that are in there are mostly at run startup and shutdown. The ones
that can occur intra-run are associated with
* the seldom-used log-each-transaction option, which pretty obviously
is a drag on performance anyway; or
* the seldom-used \sleep command, which also obviously affects pgbench's
ability to process transactions fast.

I repeat my concern that transaction rates measured with this patch will
be significantly different from those seen with the old code, and that
confusion will ensue, and that it's not hard to see how to avoid that.

regards, tom lane

On Mon, 18 Aug 2008, Tom Lane wrote:

> the seldom-used log-each-transaction option, which pretty obviously
> is a drag on performance anyway

I always recommend that people run with log each transaction turned on,
beause it's the only way to gather useful latency information. I think
runs that measure TPS without even considering that are much less useful.
The fact that it's seldom used is something I consider a problem with
pgbench.

> I repeat my concern that transaction rates measured with this patch will
> be significantly different from those seen with the old code

Last time I tried to quantify the overhead of logging with timestamps on I
couldn't even measure its impact, it was lower than the usual pgbench
noise. That was on Linux. Do you have a suggestion for a platform that
it's a problem on that I might be able to use? I'd like to do some
measurements.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

Greg Smith wrote:
> On Mon, 18 Aug 2008, Tom Lane wrote:

>> I repeat my concern that transaction rates measured with this patch will
>> be significantly different from those seen with the old code
>
> Last time I tried to quantify the overhead of logging with timestamps on
> I couldn't even measure its impact, it was lower than the usual pgbench
> noise.

There's a hardware deficiency on certain machines -- I think it's old
ones. I don't know if machines that would currently be used in
production would contain such a problem.

In any case, I think using SIGALRM as proposed by Tom is a very easy way
out of the problem.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Greg Smith wrote:
>> Last time I tried to quantify the overhead of logging with timestamps on
>> I couldn't even measure its impact, it was lower than the usual pgbench
>> noise.

> There's a hardware deficiency on certain machines -- I think it's old
> ones. I don't know if machines that would currently be used in
> production would contain such a problem.

My understanding is that it's basically "cheap PC hardware" (with clock
interfaces based on old ISA bus specs) that has the issue in a
significant way. I wouldn't expect you to see it on a serious database
server. But lots of people still do development on cheap PC hardware,
which is why I think this is worth worrying about.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> My understanding is that it's basically "cheap PC hardware" (with clock
> interfaces based on old ISA bus specs) that has the issue in a
> significant way. I wouldn't expect you to see it on a serious database
> server. But lots of people still do development on cheap PC hardware,
> which is why I think this is worth worrying about.

Ok, I rewrote the patch to use SIGALRM instead of gettimeofday.

The only thing I worried about is portability issue. POSIX functions
like alarm() or setitimer() are not available at least on Windows.
I expect alarm() is available on all platforms except Win32 and
used CreateTimerQueue() instead on Win32 in the new patch.
(We have own implementation of setitimer() in the server core, but pgbench
cannot use the function because it is a client application.)

Comments welcome and let me know if there are still some problems.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro wrote:

> The only thing I worried about is portability issue. POSIX functions
> like alarm() or setitimer() are not available at least on Windows.
> I expect alarm() is available on all platforms except Win32 and
> used CreateTimerQueue() instead on Win32 in the new patch.
> (We have own implementation of setitimer() in the server core, but pgbench
> cannot use the function because it is a client application.)

It wouldn't be unheard of to allow using timer.c into client domain
(cf. src/port/)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

"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

Hello again,

I received the following email from a helpful fellow off-list,
pointing out an error in my review:

On Fri, Sep 5, 2008 at 7:03 PM, Ragnar <gnari(at)hive(dot)is> wrote:
> On fös, 2008-09-05 at 15:07 +1000, Brendan Jurd wrote:
>> Wouldn't this be better written as:
>>
>> if ((duration > 0 && timer_exceeded) || st->cnt >= nxacts)
>> {
>> <stop>
>> }
>
> sorry, but these do not lok as the same thing to me.
>
> in the first variant there will not be a stop if
> (duration > 0) and NOT (timer_exceeded) and (st->cnt >= nxacts)
> but in the second variant there will.
>
> admittedly, i have no idea if that situation can occur.
>
> gnari
>

gnari is right. Looking closer I see that nxacts defaults to 10 in
the absence of a -t option, so my version of the code would end up
stopping when the run reaches 10 transactions, even if the user has
specified a -T option.

Sorry for the error. The (duration > 0) test does in fact need to be separate.

Thanks for the catch, gnari.

Cheers,
BJ

Here is a revised version of the pgbench duration patch.
I merged some comments from Brendan and gnari.

"Brendan Jurd" <direvus(at)gmail(dot)com> wrote:

> >> Wouldn't this be better written as:
> >> if ((duration > 0 && timer_exceeded) || st->cnt >= nxacts)
> >
> > sorry, but these do not lok as the same thing to me.
> > gnari

I used this condition expression here:
if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Mon, Sep 8, 2008 at 6:59 PM, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Here is a revised version of the pgbench duration patch.
> I merged some comments from Brendan and gnari.
>

The changes look good. I tried out the new v3 patch and didn't
encounter any problems.

One last minor quibble - I think the wording in the documentation is
still a little bit awkward:

In the first place, <emphasis>never</> believe any test that runs
for only a few seconds. Use the <literal>-t</> or <literal>-T</>
setting enough
to make the run last at least a few minutes, so as to average out noise.

This reads better IMHO if you simply omit the word "enough".

Cheers,
BJ

ITAGAKI Takahiro wrote:
> Here is a revised version of the pgbench duration patch.

Looking at the Win32 timer implementation, it's a bit different from the
one we have in src/backend/port/win32/timer.c. The one in timer.c uses a
separate thread and WaitForSingleObjectEx() to wait, while your
implementation uses CreateTimerQueue() and CreateTimerQueueTimer().
Yours seems simpler, so I wonder why the timer.c is different?

It's not too bad as it is in the patch, but it would be nice to put the
setitimer() implementation into src/port, and use the same code in the
backend as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:
> ITAGAKI Takahiro wrote:
>> Here is a revised version of the pgbench duration patch.
>
> Looking at the Win32 timer implementation, it's a bit different from the
> one we have in src/backend/port/win32/timer.c. The one in timer.c uses a
> separate thread and WaitForSingleObjectEx() to wait, while your
> implementation uses CreateTimerQueue() and CreateTimerQueueTimer().
> Yours seems simpler, so I wonder why the timer.c is different?

Probably because it was written back when we supported NT4, and the
CreateTimerQueue() stuff requires Windows 2000 to work.

> It's not too bad as it is in the patch, but it would be nice to put the
> setitimer() implementation into src/port, and use the same code in the
> backend as well.

I haven't looked at the patch ;-), but the implementation in
backend/port is tied into the signal emulation layer that's also in
backend/port, so I think doing such a move will require moving a lot
more than just the timer code...

//Magnus

ITAGAKI Takahiro wrote:
> ***************
> *** 29,36 ****
> --- 29,40 ----
> #include "postgres_fe.h"
>
> #include "libpq-fe.h"
> + #include "pqsignal.h"
>
> #include <ctype.h>
> + #include <signal.h>
> + #include <sys/time.h>
> + #include <unistd.h>
>
> #ifdef WIN32
> #undef FD_SETSIZE

sys/time.h and unistd.h are #included just a few lines after that, but
within a #ifndef WIN32 block. I don't think the patch added any
codepaths where we'd need those header files on Windows, so I presume
that was just an oversight and those two extra #includes can be removed?
I don't have a Windows environment to test it myself.

Also, should we be using pqsignal at all? It's not clear to me what it
is, to be honest, but there's a note in pqsignal.h that says "This
shouldn't be in libpq, but the monitor and some other things need it..."

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> sys/time.h and unistd.h are #included just a few lines after that, but
> within a #ifndef WIN32 block. I don't think the patch added any
> codepaths where we'd need those header files on Windows, so I presume
> that was just an oversight and those two extra #includes can be removed?
> I don't have a Windows environment to test it myself.

Well, if we did need them the buildfarm would soon tell us, no?

> Also, should we be using pqsignal at all? It's not clear to me what it
> is, to be honest, but there's a note in pqsignal.h that says "This
> shouldn't be in libpq, but the monitor and some other things need it..."

Yeah, it has more portable semantics than just plain signal().

That note isn't offbase I suppose --- now that we have src/port/ it
would have been better to put it there. But moving it now would be an
ABI break for libpq.so, and I don't think it's worth it.

Are people satisfied that the Windows part of the patch is okay?
I'm not able to review it meaningfully. One thing that looks suspicious
is that handle_sig_alarm doesn't look like it's needed for Windows;
it'd be better if win32_timer_callback just did timer_exceeded = true
for itself. (This might explain the bogus #include requirements?)

regards, tom lane

I wrote:
> Are people satisfied that the Windows part of the patch is okay?

I went ahead and applied this patch after some trivial stylistic fixes.
The buildfarm will soon tell us if the WIN32 part of the patch compiles,
but not whether it works --- would someone double-check the functioning
of the -T switch on Windows?

regards, tom lane

Tom Lane wrote:
> I wrote:
>> Are people satisfied that the Windows part of the patch is okay?
>
> I went ahead and applied this patch after some trivial stylistic fixes.
> The buildfarm will soon tell us if the WIN32 part of the patch compiles,
> but not whether it works --- would someone double-check the functioning
> of the -T switch on Windows?

Confirmed, it works fine for me. Quick look at the win32 specific code
committed, I see nothing else that would be off in that either (two
handles that are never closed, but there's no point in caring in
pgbench, since it will never happen in a loop)

//Magnus