pgbench exit code
In pgbench, when an error occurs during a benchmark run (SQL error,
connection error, etc.) it just prints an error message and then
proceeds to print the run summary normally and exits with status 0. So
this is quite inconvenient, especially from a script.
The attached patch changes this so that it exits with status 1 and also
prints a line below the summary advising that the results are incomplete.
The test suite had, curiously, encoded the previous behavior, checking
for exit status 0 vs 1 in various error situations. In this patch, I
have just updated the expected results (which are all "1" now), but
perhaps we should remove listing that individually and just check for
nonzero in all cases somewhere centrally.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-pgbench-Report-errors-during-run-better.patchtext/plain; charset=UTF-8; name=0001-pgbench-Report-errors-during-run-better.patch; x-mac-creator=0; x-mac-type=0Download
From e269dccd449efffc83e53d7f1954818b44205935 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Tue, 3 Jul 2018 19:51:24 +0200
Subject: [PATCH] pgbench: Report errors during run better
When an error occurs during a benchmark run, exit with a nonzero exit
code and write a message at the end. Previously, it would just print
the error message when it happened but then proceed to print the run
summary normally and exit with status 0.
---
src/bin/pgbench/pgbench.c | 10 +++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 51 ++++++++++----------
2 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 41b756c089..19bd6b033f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4830,6 +4830,8 @@ main(int argc, char **argv)
PGresult *res;
char *env;
+ int exit_code = 0;
+
progname = get_progname(argv[0]);
if (argc > 1)
@@ -5570,6 +5572,9 @@ main(int argc, char **argv)
(void) threadRun(thread);
#endif /* ENABLE_THREAD_SAFETY */
+ if (thread->state->state == CSTATE_ABORTED)
+ exit_code = 1;
+
/* aggregate thread level stats */
mergeSimpleStats(&stats.latency, &thread->stats.latency);
mergeSimpleStats(&stats.lag, &thread->stats.lag);
@@ -5594,7 +5599,10 @@ main(int argc, char **argv)
INSTR_TIME_SUBTRACT(total_time, start_time);
printResults(threads, &stats, total_time, conn_total_time, latency_late);
- return 0;
+ if (exit_code != 0)
+ fprintf(stderr, "Run was aborted; the above results are incomplete.\n");
+
+ return exit_code;
}
static void *
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 2fc021dde7..82e75c5f03 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -534,7 +534,7 @@ sub pgbench
# SQL
[
'sql syntax error',
- 0,
+ 1,
[
qr{ERROR: syntax error},
qr{prepared statement .* does not exist}
@@ -553,11 +553,11 @@ sub pgbench
# SHELL
[
- 'shell bad command', 0,
+ 'shell bad command', 1,
[qr{\(shell\) .* meta-command failed}], q{\shell no-such-command}
],
[
- 'shell undefined variable', 0,
+ 'shell undefined variable', 1,
[qr{undefined variable ":nosuchvariable"}],
q{-- undefined variable in shell
\shell echo ::foo :nosuchvariable
@@ -597,81 +597,81 @@ sub pgbench
[qr{unexpected function name}], q{\set i noSuchFunction()}
],
[
- 'set invalid variable name', 0,
+ 'set invalid variable name', 1,
[qr{invalid variable name}], q{\set . 1}
],
[
- 'set int overflow', 0,
+ 'set int overflow', 1,
[qr{double to int overflow for 100}], q{\set i int(1E32)}
],
- [ 'set division by zero', 0, [qr{division by zero}], q{\set i 1/0} ],
+ [ 'set division by zero', 1, [qr{division by zero}], q{\set i 1/0} ],
[
- 'set bigint out of range', 0,
+ 'set bigint out of range', 1,
[qr{bigint out of range}], q{\set i 9223372036854775808 / -1}
],
[
'set undefined variable',
- 0,
+ 1,
[qr{undefined variable "nosuchvariable"}],
q{\set i :nosuchvariable}
],
[ 'set unexpected char', 1, [qr{unexpected character .;.}], q{\set i ;} ],
[
'set too many args',
- 0,
+ 1,
[qr{too many function arguments}],
q{\set i least(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16)}
],
[
- 'set empty random range', 0,
+ 'set empty random range', 1,
[qr{empty range given to random}], q{\set i random(5,3)}
],
[
'set random range too large',
- 0,
+ 1,
[qr{random range is too large}],
q{\set i random(-9223372036854775808, 9223372036854775807)}
],
[
'set gaussian param too small',
- 0,
+ 1,
[qr{gaussian param.* at least 2}],
q{\set i random_gaussian(0, 10, 1.0)}
],
[
'set exponential param greater 0',
- 0,
+ 1,
[qr{exponential parameter must be greater }],
q{\set i random_exponential(0, 10, 0.0)}
],
[
'set zipfian param to 1',
- 0,
+ 1,
[qr{zipfian parameter must be in range \(0, 1\) U \(1, \d+\]}],
q{\set i random_zipfian(0, 10, 1)}
],
[
'set zipfian param too large',
- 0,
+ 1,
[qr{zipfian parameter must be in range \(0, 1\) U \(1, \d+\]}],
q{\set i random_zipfian(0, 10, 1000000)}
],
[
- 'set non numeric value', 0,
+ 'set non numeric value', 1,
[qr{malformed variable "foo" value: "bla"}], q{\set i :foo + 1}
],
[ 'set no expression', 1, [qr{syntax error}], q{\set i} ],
[ 'set missing argument', 1, [qr{missing argument}i], q{\set} ],
[
- 'set not a bool', 0,
+ 'set not a bool', 1,
[qr{cannot coerce double to boolean}], q{\set b NOT 0.0}
],
[
- 'set not an int', 0,
+ 'set not an int', 1,
[qr{cannot coerce boolean to int}], q{\set i TRUE + 2}
],
[
- 'set not a double', 0,
+ 'set not a double', 1,
[qr{cannot coerce boolean to double}], q{\set d ln(TRUE)}
],
[
@@ -681,7 +681,7 @@ sub pgbench
q{\set i CASE TRUE THEN 1 ELSE 0 END}
],
[
- 'set random error', 0,
+ 'set random error', 1,
[qr{cannot coerce boolean to int}], q{\set b random(FALSE, TRUE)}
],
[
@@ -695,18 +695,18 @@ sub pgbench
# SETSHELL
[
- 'setshell not an int', 0,
+ 'setshell not an int', 1,
[qr{command must return an integer}], q{\setshell i echo -n one}
],
[ 'setshell missing arg', 1, [qr{missing argument }], q{\setshell var} ],
[
- 'setshell no such command', 0,
+ 'setshell no such command', 1,
[qr{could not read result }], q{\setshell var no-such-command}
],
# SLEEP
[
- 'sleep undefined variable', 0,
+ 'sleep undefined variable', 1,
[qr{sleep: undefined variable}], q{\sleep :nosuchvariable}
],
[
@@ -729,7 +729,7 @@ sub pgbench
],
[ 'misc empty script', 1, [qr{empty command list for script}], q{} ],
[
- 'bad boolean', 0,
+ 'bad boolean', 1,
[qr{malformed variable.*trueXXX}], q{\set b :badtrue or true}
],);
@@ -737,12 +737,13 @@ sub pgbench
for my $e (@errors)
{
my ($name, $status, $re, $script) = @$e;
+ $status != 0 or die;
my $n = '001_pgbench_error_' . $name;
$n =~ s/ /_/g;
pgbench(
'-n -t 1 -Dfoo=bla -Dnull=null -Dtrue=true -Done=1 -Dzero=0.0 -Dbadtrue=trueXXX -M prepared',
$status,
- [ $status ? qr{^$} : qr{processed: 0/1} ],
+ [],
$re,
'pgbench script error: ' . $name,
{ $n => $script });
--
2.18.0
Hello Peter,
In pgbench, when an error occurs during a benchmark run (SQL error,
connection error, etc.) it just prints an error message and then
proceeds to print the run summary normally and exits with status 0. So
this is quite inconvenient, especially from a script.
Yep. I'm fine with changing this behavior... last time I suggested
something like that, or probably something more drastic like existing
immediately when there is little sense to go on, it was not approved, some
people want the report anyway.
Your approach of not changing the output too much but changing the exit
status and adding a warning may get through more easily.
Note that there is some logic in distinguishing between different type of
errors (before the bench start vs the bench has started), so I'd suggest
that the exit status should be different.
The attached patch changes this so that it exits with status 1 and also
prints a line below the summary advising that the results are incomplete.The test suite had, curiously, encoded the previous behavior,
Sure, the point of the tests is to check for unwanted regressions. It does
not mean that the behavior is ideal for all use cases.
Patch applies cleanly, compiles, global & local "make check" are ok.
The abort is by a client, but the code seems to only check the first
client of a thread. ISTM that if the second or later client abort it may
not be detected? Probably an intermediate aggregation at the thread level
is needed, or maybe a global variable, or as errors are counted somewhere,
it may be enough just to check that the count is non zero?
- [ $status ? qr{^$} : qr{processed: 0/1} ],
+ [],
The patch removes a check that there was an output and that no
transactions where processed. ISTM it should be kept. If a different exit
status is chosen on abort, that would allow to keep it easily.
Probably there should be some documentation changes as well?
Note that the patch probably interferes in small ways with Marina's patch
for retrying on serialization or deadlock errors, see:
https://commitfest.postgresql.org/18/1645/
--
Fabien.
The attached patch changes this so that it exits with status 1 and also
prints a line below the summary advising that the results are incomplete.
BTW, I have added this patch to the next CF:
https://commitfest.postgresql.org/19/1751/
--
Fabien.
On Fri, Aug 10, 2018 at 01:50:49PM +0200, Fabien COELHO wrote:
BTW, I have added this patch to the next CF:
The latest patch does not apply anymore. Peter, could you rebase it?
--
Michael
On Mon, Oct 01, 2018 at 11:29:19AM +0900, Michael Paquier wrote:
On Fri, Aug 10, 2018 at 01:50:49PM +0200, Fabien COELHO wrote:
BTW, I have added this patch to the next CF:
The latest patch does not apply anymore. Peter, could you rebase it?
On top of that, I just noticed that the patch has been waiting for input
from the author for a couple of weeks, so the thing is returned with
feedback for now.
--
Michael
On 10/08/2018 01:41, Fabien COELHO wrote:
Your approach of not changing the output too much but changing the exit
status and adding a warning may get through more easily.Note that there is some logic in distinguishing between different type of
errors (before the bench start vs the bench has started), so I'd suggest
that the exit status should be different.
done
The abort is by a client, but the code seems to only check the first
client of a thread. ISTM that if the second or later client abort it may
not be detected? Probably an intermediate aggregation at the thread level
is needed, or maybe a global variable, or as errors are counted somewhere,
it may be enough just to check that the count is non zero?
fixed
- [ $status ? qr{^$} : qr{processed: 0/1} ], + [],The patch removes a check that there was an output and that no
transactions where processed. ISTM it should be kept. If a different exit
status is chosen on abort, that would allow to keep it easily.
fixed
Probably there should be some documentation changes as well?
done
New patched attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-pgbench-Report-errors-during-run-better.patchtext/plain; charset=UTF-8; name=v2-0001-pgbench-Report-errors-during-run-better.patch; x-mac-creator=0; x-mac-type=0Download
From 1cab742954a625134fa1b32d5e008c32d7a9e7ee Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Tue, 9 Oct 2018 12:04:04 +0200
Subject: [PATCH v2] pgbench: Report errors during run better
When an error occurs during a benchmark run, exit with a nonzero exit
code and write a message at the end. Previously, it would just print
the error message when it happened but then proceed to print the run
summary normally and exit with status 0. To still allow
distinguishing setup from run-time errors, we use exit status 2 for
the new state, whereas existing errors during pgbench initialization
use exit status 1.
---
doc/src/sgml/ref/pgbench.sgml | 11 ++++
src/bin/pgbench/pgbench.c | 11 +++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 57 ++++++++++----------
3 files changed, 50 insertions(+), 29 deletions(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 8c464c9d42..03d92ff451 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -804,6 +804,17 @@ <title>Common Options</title>
</refsect2>
</refsect1>
+ <refsect1>
+ <title>Exit Status</title>
+
+ <para>
+ A successful run with exit with status 0. Exit status 1 indicates problems
+ such as invalid command-line options. Errors during the run such as
+ database errors or problems in the script result in exit status 2. In the
+ latter case, <application>pgbench</application> will print partial results.
+ </para>
+ </refsect1>
+
<refsect1>
<title>Notes</title>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 436764b9c9..81bc6d8a6e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4931,6 +4931,8 @@ main(int argc, char **argv)
PGresult *res;
char *env;
+ int exit_code = 0;
+
progname = get_progname(argv[0]);
if (argc > 1)
@@ -5675,6 +5677,10 @@ main(int argc, char **argv)
(void) threadRun(thread);
#endif /* ENABLE_THREAD_SAFETY */
+ for (int j = 0; j < thread->nstate; j++)
+ if (thread->state[j].state == CSTATE_ABORTED)
+ exit_code = 2;
+
/* aggregate thread level stats */
mergeSimpleStats(&stats.latency, &thread->stats.latency);
mergeSimpleStats(&stats.lag, &thread->stats.lag);
@@ -5699,7 +5705,10 @@ main(int argc, char **argv)
INSTR_TIME_SUBTRACT(total_time, start_time);
printResults(threads, &stats, total_time, conn_total_time, latency_late);
- return 0;
+ if (exit_code != 0)
+ fprintf(stderr, "Run was aborted; the above results are incomplete.\n");
+
+ return exit_code;
}
static void *
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index d972903f2a..0081989026 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -537,7 +537,7 @@ sub pgbench
# SQL
[
'sql syntax error',
- 0,
+ 2,
[
qr{ERROR: syntax error},
qr{prepared statement .* does not exist}
@@ -556,11 +556,11 @@ sub pgbench
# SHELL
[
- 'shell bad command', 0,
+ 'shell bad command', 2,
[qr{\(shell\) .* meta-command failed}], q{\shell no-such-command}
],
[
- 'shell undefined variable', 0,
+ 'shell undefined variable', 2,
[qr{undefined variable ":nosuchvariable"}],
q{-- undefined variable in shell
\shell echo ::foo :nosuchvariable
@@ -600,75 +600,75 @@ sub pgbench
[qr{unexpected function name}], q{\set i noSuchFunction()}
],
[
- 'set invalid variable name', 0,
+ 'set invalid variable name', 2,
[qr{invalid variable name}], q{\set . 1}
],
[
- 'set division by zero', 0,
+ 'set division by zero', 2,
[qr{division by zero}], q{\set i 1/0}
],
[ 'set undefined variable',
- 0,
+ 2,
[qr{undefined variable "nosuchvariable"}],
q{\set i :nosuchvariable}
],
[ 'set unexpected char', 1, [qr{unexpected character .;.}], q{\set i ;} ],
[
'set too many args',
- 0,
+ 2,
[qr{too many function arguments}],
q{\set i least(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16)}
],
[
- 'set empty random range', 0,
+ 'set empty random range', 2,
[qr{empty range given to random}], q{\set i random(5,3)}
],
[
'set random range too large',
- 0,
+ 2,
[qr{random range is too large}],
q{\set i random(:minint, :maxint)}
],
[
'set gaussian param too small',
- 0,
+ 2,
[qr{gaussian param.* at least 2}],
q{\set i random_gaussian(0, 10, 1.0)}
],
[
'set exponential param greater 0',
- 0,
+ 2,
[qr{exponential parameter must be greater }],
q{\set i random_exponential(0, 10, 0.0)}
],
[
'set zipfian param to 1',
- 0,
+ 2,
[qr{zipfian parameter must be in range \(0, 1\) U \(1, \d+\]}],
q{\set i random_zipfian(0, 10, 1)}
],
[
'set zipfian param too large',
- 0,
+ 2,
[qr{zipfian parameter must be in range \(0, 1\) U \(1, \d+\]}],
q{\set i random_zipfian(0, 10, 1000000)}
],
[
- 'set non numeric value', 0,
+ 'set non numeric value', 2,
[qr{malformed variable "foo" value: "bla"}], q{\set i :foo + 1}
],
[ 'set no expression', 1, [qr{syntax error}], q{\set i} ],
[ 'set missing argument', 1, [qr{missing argument}i], q{\set} ],
[
- 'set not a bool', 0,
+ 'set not a bool', 2,
[qr{cannot coerce double to boolean}], q{\set b NOT 0.0}
],
[
- 'set not an int', 0,
+ 'set not an int', 2,
[qr{cannot coerce boolean to int}], q{\set i TRUE + 2}
],
[
- 'set not a double', 0,
+ 'set not a double', 2,
[qr{cannot coerce boolean to double}], q{\set d ln(TRUE)}
],
[
@@ -678,7 +678,7 @@ sub pgbench
q{\set i CASE TRUE THEN 1 ELSE 0 END}
],
[
- 'set random error', 0,
+ 'set random error', 2,
[qr{cannot coerce boolean to int}], q{\set b random(FALSE, TRUE)}
],
[
@@ -691,31 +691,31 @@ sub pgbench
],
# SET: ARITHMETIC OVERFLOW DETECTION
- [ 'set double to int overflow', 0,
+ [ 'set double to int overflow', 2,
[ qr{double to int overflow for 100} ], q{\set i int(1E32)} ],
- [ 'set bigint add overflow', 0,
+ [ 'set bigint add overflow', 2,
[ qr{int add out} ], q{\set i (1<<62) + (1<<62)} ],
- [ 'set bigint sub overflow', 0,
+ [ 'set bigint sub overflow', 2,
[ qr{int sub out} ], q{\set i 0 - (1<<62) - (1<<62) - (1<<62)} ],
- [ 'set bigint mul overflow', 0,
+ [ 'set bigint mul overflow', 2,
[ qr{int mul out} ], q{\set i 2 * (1<<62)} ],
- [ 'set bigint div out of range', 0,
+ [ 'set bigint div out of range', 2,
[ qr{bigint div out of range} ], q{\set i :minint / -1} ],
# SETSHELL
[
- 'setshell not an int', 0,
+ 'setshell not an int', 2,
[qr{command must return an integer}], q{\setshell i echo -n one}
],
[ 'setshell missing arg', 1, [qr{missing argument }], q{\setshell var} ],
[
- 'setshell no such command', 0,
+ 'setshell no such command', 2,
[qr{could not read result }], q{\setshell var no-such-command}
],
# SLEEP
[
- 'sleep undefined variable', 0,
+ 'sleep undefined variable', 2,
[qr{sleep: undefined variable}], q{\sleep :nosuchvariable}
],
[
@@ -738,7 +738,7 @@ sub pgbench
],
[ 'misc empty script', 1, [qr{empty command list for script}], q{} ],
[
- 'bad boolean', 0,
+ 'bad boolean', 2,
[qr{malformed variable.*trueXXX}], q{\set b :badtrue or true}
],);
@@ -746,13 +746,14 @@ sub pgbench
for my $e (@errors)
{
my ($name, $status, $re, $script) = @$e;
+ $status != 0 or die "invalid expected status for test \"$name\"";
my $n = '001_pgbench_error_' . $name;
$n =~ s/ /_/g;
pgbench(
'-n -t 1 -M prepared -Dfoo=bla -Dnull=null -Dtrue=true -Done=1 -Dzero=0.0 ' .
'-Dbadtrue=trueXXX -Dmaxint=9223372036854775807 -Dminint=-9223372036854775808',
$status,
- [ $status ? qr{^$} : qr{processed: 0/1} ],
+ [ $status == 1 ? qr{^$} : qr{processed: 0/1} ],
$re,
'pgbench script error: ' . $name,
{ $n => $script });
base-commit: eee01d606eb40f760799320e75d45c84022353d8
--
2.19.0
Hello Peter,
The abort is by a client, but the code seems to only check the first
client of a thread. ISTM that if the second or later client abort it may
not be detected? Probably an intermediate aggregation at the thread level
is needed, or maybe a global variable, or as errors are counted somewhere,
it may be enough just to check that the count is non zero?fixed
With an aggregation. Fine with me.
The patch removes a check that there was an output and that no
transactions where processed. ISTM it should be kept. If a different exit
status is chosen on abort, that would allow to keep it easily.fixed
Ok.
Probably there should be some documentation changes as well?
done
Ok.
Patch applies cleanly, compiles, global & local "make check" are ok.
Doc build is okay.
I noticed "for (int i"… pgbench is welcome to C99:-)
No further remarks. I turned the patch as ready on the CF app.
Attached a file I used for some manual tests.
--
Fabien.
Attachments:
On 12/10/2018 21:22, Fabien COELHO wrote:
No further remarks. I turned the patch as ready on the CF app.
Committed, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services