pgbench small bug fix
While testing for something else I encountered two small bugs under very
low rate (--rate=0.1). The attached patches fixes these.
- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).
- when there is a progress (-P) report, ensure that all progress
reports are shown even if no more transactions are schedule.
--
Fabien.
Attachments:
pgbench-duration-under-rate-1.patchtext/x-diff; name=pgbench-duration-under-rate-1.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d5f242c..6cd6500 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1195,6 +1195,12 @@ top:
thread->throttle_trigger += wait;
st->txn_scheduled = thread->throttle_trigger;
+ /* stop client if next transaction is beyond pgbench end of execution */
+ if (duration &&
+ (st->txn_scheduled / 1000000.0) >
+ INSTR_TIME_GET_DOUBLE(thread->start_time) + duration)
+ return false;
+
/*
* If this --latency-limit is used, and this slot is already late so
* that the transaction will miss the latency limit even if it
@@ -3674,7 +3680,10 @@ threadRun(void *arg)
}
}
- while (remains > 0)
+ while (remains > 0 ||
+ /* thread zero keeps on printing progress report if any */
+ (progress && thread->tid == 0 && duration &&
+ next_report <= thread_start + (int64) duration * 1000000))
{
fd_set input_mask;
int maxsock; /* max socket number to be waited */
On Wed, Jan 27, 2016 at 2:31 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
While testing for something else I encountered two small bugs under very low
rate (--rate=0.1). The attached patches fixes these.- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).
Why does this use INSTR_TIME_GET_DOUBLE() and not INSTR_TIME_GET_MICROSEC()?
Also, why do we really need this change? Won't the timer expiration
stop the thread at the right time anyway? I mean, sure, in theory
it's wasteful for the thread to sit around doing nothing waiting for
the timer to expire, but it's not evident to me that hurts anything,
really.
- when there is a progress (-P) report, ensure that all progress
reports are shown even if no more transactions are schedule.
That's pretty ugly - it would be easy for the test at the top of the
loop to be left out of sync with the similar test inside the loop by
some future patch. And again, I wonder why this is really a bug.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
<Ooops, resent, wrong "From" again... sorry :-( >
Hello Robert,
While testing for something else I encountered two small bugs under very low
rate (--rate=0.1). The attached patches fixes these.- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).Why does this use INSTR_TIME_GET_DOUBLE() and not INSTR_TIME_GET_MICROSEC()?
I choose that because duration is in seconds, but MICROSEC would work fine as
well. See attached version.
Also, why do we really need this change? Won't the timer expiration
stop the thread at the right time anyway?
Not always. When several threads are used, the time expiration in non-zero
threads is currently triggered after the last schedule transaction, even if this
transaction is long after the expiration, which means it runs overtime:
sh> time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2
starting vacuum...end.
progress: 1.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms
progress: 2.0 s, 1.0 tps, lat 12.129 ms stddev 0.000, lag 1.335 ms
progress: 3.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms
progress: 4.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms
progress: 5.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms
< 6 seconds wait for a schedule transaction in thread 1
no report because thread 0 expired...>
transaction type: TPC-B (sort of)
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 2
duration: 5 s
number of transactions actually processed: 2
latency average: 14.648 ms
latency stddev: 2.518 ms
rate limit schedule lag: avg 5.598 (max 9.861) ms
tps = 0.180517 (including connections establishing)
tps = 0.180566 (excluding connections establishing)
real 0m11.158s
^^ 11 seconds, not 5.
user 0m0.041s
sys 0m0.013s
I mean, sure, in theory it's wasteful for the thread to sit around doing
nothing waiting for the timer to expire, but it's not evident to me that hurts
anything, really.
I compute stats on the output, including the progress report, to check for
responsiveness (eg it is not stuck somewhere because of a checkpoint which
triggered some IO storm or whatever), for that purpose it is better for the
trace to be there as expected.
- when there is a progress (-P) report, ensure that all progress
reports are shown even if no more transactions are schedule.That's pretty ugly - it would be easy for the test at the top of the
loop to be left out of sync with the similar test inside the loop by
some future patch.
Hmmm. Cannot help it.
A cleaner fix would be to have the main thread do only the progress and periodic
stats aggregation, while other threads would do actual transactions, however
that would break pgbench working without threads, so I think this is a no go.
And again, I wonder why this is really a bug.
Well, if you are fine to set "-T 5 -P 1" and the run not to last 5 seconds nor
print any report every second, then it is not a bug:
sh> time ./pgbench -T 5 -P 1 -R 0.1 -c 2 -j 2
starting vacuum...end.
< UNLUCKY, NO PROGRESS REPORT AT ALL...>
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 2
duration: 5 s
number of transactions actually processed: 1
latency average = 16.198 ms
latency stddev = 0.000 ms
rate limit schedule lag: avg 4.308 (max 4.308) ms
tps = 0.236361 (including connections establishing)
tps = 0.236771 (excluding connections establishing)
real 0m4.256s
--
Fabien.
Attachments:
pgbench-duration-under-rate-2.patchtext/x-diff; name=pgbench-duration-under-rate-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7eb6a2d..08d358f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1285,6 +1285,12 @@ top:
thread->throttle_trigger += wait;
st->txn_scheduled = thread->throttle_trigger;
+ /* stop client if next transaction is beyond pgbench end of execution */
+ if (duration &&
+ st->txn_scheduled >
+ INSTR_TIME_GET_MICROSEC(thread->start_time) + (int64) 1000000 * duration)
+ return false;
+
/*
* If this --latency-limit is used, and this slot is already late so
* that the transaction will miss the latency limit even if it
@@ -3640,7 +3646,10 @@ threadRun(void *arg)
}
}
- while (remains > 0)
+ while (remains > 0 ||
+ /* thread zero keeps on printing progress report if any */
+ (progress && thread->tid == 0 && duration &&
+ next_report <= thread_start + (int64) duration * 1000000))
{
fd_set input_mask;
int maxsock; /* max socket number to be waited */
time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2
On my laptop this command executes 25 seconds instead of 5. I'm pretty
sure it IS a bug. Probably a minor one though.
I tested this patch on Ubuntu 14.04 LTS with GCC 4.8. It applies
cleanly on master branch (c7111d11) and solves a described problem.
No compilation warnings. Everything else works as before.
Still I wonder if code could be patched more cleanly. Instead of:
if(someint)
if(somebool)
... you should probably write:
if(someint > 0)
if(somebool == TRUE)
Also I suggest to introduce a few new boolean variables with meaningful
names instead of writing all these long expressions right inside of
if( ... ).
As a side note I noticed that pgbench.c is not pgindent'ed. Since you
are modifying this file anyway probably you cold solve this issue too?
As a separate patch perhaps.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 3, 2016 at 7:23 AM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> wrote:
time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2
On my laptop this command executes 25 seconds instead of 5. I'm pretty
sure it IS a bug. Probably a minor one though.I tested this patch on Ubuntu 14.04 LTS with GCC 4.8. It applies
cleanly on master branch (c7111d11) and solves a described problem.
No compilation warnings. Everything else works as before.Still I wonder if code could be patched more cleanly. Instead of:
if(someint)
if(somebool)... you should probably write:
if(someint > 0)
if(somebool == TRUE)
I think our usual style is to test Booleans directly; that is if
(somebool). But for other types, we typically include an explicit
comparison, like if (someint != 0) or if (someint > 0).
As a side note I noticed that pgbench.c is not pgindent'ed. Since you
are modifying this file anyway probably you cold solve this issue too?
As a separate patch perhaps.
That's not really this patch's job.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello Aleksander,
Thanks for the look at the patch.
time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2
On my laptop this command executes 25 seconds instead of 5.
I'm pretty sure it IS a bug. Probably a minor one though.
Sure.
[...] you should probably write:
if(someint > 0)
Ok.
if(somebool == TRUE)
I like "if (somebool)", the "== TRUE" looks like a tautology, and the
short version is also the current practice in the project.
Also I suggest to introduce a few new boolean variables with meaningful
names instead of writing all these long expressions right inside of
if( ... ).
I agree about the lisibility, but there are semantics issues to consider:
if (short-A && pretty-long-B)
If short-A is false then pretty-long-B is not evaluated, which is a win
because it also costs, I try to order conditions... If I move
pretty-long-B before then the cost is always paid. Now I could write:
if (short-A) {
bool b = pretty-long-B;
if (b) {
...
But this looks contrived and people would raise other questions about such
a strange construct for implementing && in 3 lines, 2 if and 1 variable...
As a side note I noticed that pgbench.c is not pgindent'ed. Since you
are modifying this file anyway probably you cold solve this issue too?
As a separate patch perhaps.
As Robert said, not the purpose of this patch.
Attached is a v3 which test integers more logically. I'm a lazy programmer
who tends to minimize the number of key strokes.
--
Fabien.
Attachments:
pgbench-duration-under-rate-3.patchtext/x-diff; name=pgbench-duration-under-rate-3.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 66cfdc9..82707ed 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1362,6 +1362,12 @@ top:
thread->throttle_trigger += wait;
st->txn_scheduled = thread->throttle_trigger;
+ /* stop client if next transaction is beyond pgbench end of execution */
+ if (duration > 0 &&
+ st->txn_scheduled >
+ INSTR_TIME_GET_MICROSEC(thread->start_time) + (int64) 1000000 * duration)
+ return false;
+
/*
* If this --latency-limit is used, and this slot is already late so
* that the transaction will miss the latency limit even if it
@@ -3724,7 +3730,10 @@ threadRun(void *arg)
}
}
- while (remains > 0)
+ while (remains > 0 ||
+ /* thread zero keeps on printing progress report if any */
+ (progress > 0 && thread->tid == 0 && duration > 0 &&
+ next_report <= thread_start + (int64) duration * 1000000))
{
fd_set input_mask;
int maxsock; /* max socket number to be waited */
Attached is a v3 which test integers more logically. I'm a lazy
programmer who tends to minimize the number of key strokes.
Well. From what I can tell this patch is Ready for Committer.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Aleksander Alekseev wrote:
Attached is a v3 which test integers more logically. I'm a lazy
programmer who tends to minimize the number of key strokes.Well. From what I can tell this patch is Ready for Committer.
I'm not a fan of this approach either. Would it be too complicated if
we had a global variable that indicates which thread is the progress
reporter? We start that with thread 0, but if the reporter thread
finishes its transactions then it elects some other thread which hasn't
yet finished. For this to work, each thread would have to maintain in a
global variable whether it has finished or not.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello Alvaro,
Attached is a v3 which test integers more logically. I'm a lazy
programmer who tends to minimize the number of key strokes.Well. From what I can tell this patch is Ready for Committer.
I'm not a fan of this approach either. Would it be too complicated if
we had a global variable that indicates which thread is the progress
reporter? We start that with thread 0, but if the reporter thread
finishes its transactions then it elects some other thread which hasn't
yet finished. For this to work, each thread would have to maintain in a
global variable whether it has finished or not.
Hmmm.
Probably it is possible, but it will sure need more that one little
condition to be achieved... I do not think that introducing a non trivial
distributed election algorithm involving locks and so would be a good
decision for this very little matter.
My advice is "keep it simple".
If this is a blocker, I can sure write such an algorithm, when I have some
spare time, but I'm not sure that the purpose is worth it.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabien COELHO wrote:
Probably it is possible, but it will sure need more that one little
condition to be achieved... I do not think that introducing a non trivial
distributed election algorithm involving locks and so would be a good
decision for this very little matter.My advice is "keep it simple".
If this is a blocker, I can sure write such an algorithm, when I have some
spare time, but I'm not sure that the purpose is worth it.
You're probably right, but TBH I'm pretty unsure about this whole thing.
I will leave it alone for the time being.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Probably it is possible, but it will sure need more that one little
condition to be achieved... I do not think that introducing a non trivial
distributed election algorithm involving locks and so would be a good
decision for this very little matter.My advice is "keep it simple".
If this is a blocker, I can sure write such an algorithm, when I have some
spare time, but I'm not sure that the purpose is worth it.You're probably right, but TBH I'm pretty unsure about this whole thing.
If the question is "is there a bug", then answer is yes. The progress
report may disappear if thread 0 happens to stop, even of all other
threads go on. Obviously it only concerns slow queries, but there is no
reason why pgbench should not work with slow queries. I can imagin good
reason to do that, say to check the impact of such queries on an OLTP
load.
The bug can be kept instead, and it can be called a feature.
I will leave it alone for the time being.
Maybe you could consider pushing the first part of the patch, which stops
if a transaction is scheduled after the end of the run? Or is this part
bothering you as well?
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabien COELHO wrote:
Probably it is possible, but it will sure need more that one little
condition to be achieved... I do not think that introducing a non trivial
distributed election algorithm involving locks and so would be a good
decision for this very little matter.My advice is "keep it simple".
If this is a blocker, I can sure write such an algorithm, when I have some
spare time, but I'm not sure that the purpose is worth it.You're probably right, but TBH I'm pretty unsure about this whole thing.
If the question is "is there a bug", then answer is yes. The progress report
may disappear if thread 0 happens to stop, even of all other threads go on.
Obviously it only concerns slow queries, but there is no reason why pgbench
should not work with slow queries. I can imagin good reason to do that, say
to check the impact of such queries on an OLTP load.The bug can be kept instead, and it can be called a feature.
No, I agree that this looks like a bug and that we should fix it; for
example, if all connections from thread 0 terminate for some reason,
there will be no more reports, even if the other threads continue.
That's bad too.
What I'm unsure about is the proposed fix.
I will leave it alone for the time being.
Maybe you could consider pushing the first part of the patch, which stops if
a transaction is scheduled after the end of the run? Or is this part
bothering you as well?
So there are *two* bugs here?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
You're probably right, but TBH I'm pretty unsure about this whole thing.
If the question is "is there a bug", then answer is yes. The progress report
may disappear if thread 0 happens to stop, even of all other threads go on.
Obviously it only concerns slow queries, but there is no reason why pgbench
should not work with slow queries. I can imagin good reason to do that, say
to check the impact of such queries on an OLTP load.The bug can be kept instead, and it can be called a feature.
No, I agree that this looks like a bug and that we should fix it; for
example, if all connections from thread 0 terminate for some reason,
there will be no more reports, even if the other threads continue.
That's bad too.What I'm unsure about is the proposed fix.
I will leave it alone for the time being.
Maybe you could consider pushing the first part of the patch, which stops if
a transaction is scheduled after the end of the run? Or is this part
bothering you as well?So there are *two* bugs here?
Hmmm... AFAICR, maybe fixing the first creates the second issue, i.e.
maybe the second issue is currently hidden by the thread going on after
the end of the run, so the second is just a latent bug that cannot be
encountered.
I'm not sure whether I'm very clear:-)
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 27, 2016 at 2:31 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).
Every other place where doCustom() returns false is implemented as
return clientDone(...). I think this should probably do the same.
I also think that we should probably store the end time someplace
instead of recomputing it in multiple places (this patch adds two such
places).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 27, 2016 at 2:31 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).Every other place where doCustom() returns false is implemented as
return clientDone(...). I think this should probably do the same.
Why not. clientDone() second arguments is totally ignored, I put true
because it looks better.
I also think that we should probably store the end time someplace
instead of recomputing it in multiple places (this patch adds two such
places).
Why not.
Attached is a v4.
--
Fabien.
Attachments:
pgbench-duration-under-rate-4.patchtext/x-diff; name=pgbench-duration-under-rate-4.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 92df750..b89f5ad 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -94,6 +94,7 @@ static int pthread_join(pthread_t th, void **thread_return);
int nxacts = 0; /* number of transactions per client */
int duration = 0; /* duration in seconds */
+int64 end_time = 0; /* when to stop in micro seconds, under -T */
/*
* scaling factor. for example, scale = 10 will make 1000000 tuples in
@@ -1362,6 +1363,10 @@ top:
thread->throttle_trigger += wait;
st->txn_scheduled = thread->throttle_trigger;
+ /* stop client if next transaction is beyond pgbench end of execution */
+ if (duration > 0 && st->txn_scheduled > end_time)
+ return clientDone(st, true);
+
/*
* If this --latency-limit is used, and this slot is already late so
* that the transaction will miss the latency limit even if it
@@ -3582,6 +3587,11 @@ main(int argc, char **argv)
INSTR_TIME_SET_CURRENT(thread->start_time);
+ /* compute when to stop */
+ if (duration > 0)
+ end_time = INSTR_TIME_GET_MICROSEC(thread->start_time) +
+ (int64) 1000000 * duration;
+
/* the first thread (i = 0) is executed by main thread */
if (i > 0)
{
@@ -3600,6 +3610,10 @@ main(int argc, char **argv)
}
#else
INSTR_TIME_SET_CURRENT(threads[0].start_time);
+ /* compute when to stop */
+ if (duration > 0)
+ end_time = INSTR_TIME_GET_MICROSEC(threads[0].start_time) +
+ (int64) 1000000 * duration;
threads[0].thread = INVALID_THREAD;
#endif /* ENABLE_THREAD_SAFETY */
@@ -3738,7 +3752,10 @@ threadRun(void *arg)
}
}
- while (remains > 0)
+ while (remains > 0 ||
+ /* thread zero keeps on printing progress report if any */
+ (progress > 0 && thread->tid == 0 && duration > 0 &&
+ next_report <= end_time))
{
fd_set input_mask;
int maxsock; /* max socket number to be waited */
On Wed, Mar 9, 2016 at 4:12 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
On Wed, Jan 27, 2016 at 2:31 PM, Fabien COELHO <coelho@cri.ensmp.fr>
wrote:- when a duration (-T) is specified, ensure that pgbench ends at that
time (i.e. do not wait for a transaction beyond the end of the run).Every other place where doCustom() returns false is implemented as
return clientDone(...). I think this should probably do the same.Why not. clientDone() second arguments is totally ignored, I put true
because it looks better.I also think that we should probably store the end time someplace
instead of recomputing it in multiple places (this patch adds two such
places).Why not.
Attached is a v4.
OK, I've committed the fix for the -T part. It didn't back-patch
cleanly, and it is a minor bug, so I'm not inclined to worry about it
further.
I didn't commit the fix for the -P part, because Alvaro objected to
the proposed method of fixing it as ugly, and I think he's right.
Unless you can come up with a nicer-looking fix, I think that part is
going to stay unfixed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
OK, I've committed the fix for the -T part. It didn't back-patch
cleanly, and it is a minor bug, so I'm not inclined to worry about it
further.
I agree that it is a very minor bug and not necessary worth back-patching.
I didn't commit the fix for the -P part, because Alvaro objected to
the proposed method of fixing it as ugly, and I think he's right.
Unless you can come up with a nicer-looking fix, I think that part is
going to stay unfixed.
A bug kept on esthetical ground, that's a first!
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers