On Wed, 9 Aug 2023 10:46:38 +0900
Yugo NAGATA <nag...@sraoss.co.jp> wrote:
> On Wed, 9 Aug 2023 02:15:01 +0200 (CEST)
> Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>
> >
> > Hello Yugo-san,
> >
> > > There are cases where "goto done" is used where some error like
> > > "invalid socket: ..." happens. I would like to make pgbench exit in
> > > such cases, too, so I chose to handle errors below the "done:" label.
> > > However, we can change it to call "exit" instead of "goo done" at each
> > > place. Do you think this is better?
> >
> > Good point.
> >
> > Now I understand the "!= FINISHED", because indeed in these cases the done
> > is reached with unfinished but not necessarily ABORTED clients, and the
> > comment was somehow misleading.
> >
> > On reflection, there should be only one exit() call, thus I'd say to keep
> > the "goto done" as you did, but to move the checking loop *before* the
> > disconnect_all, and the overall section comment could be something like
> > "possibly abort if any client is not finished, meaning some error
> > occured", which is consistent with the "!= FINISHED" condition.
>
> Thank you for your suggestion.
> I'll fix as above and submit a updated patch soon.
I attached the updated patch v3 including changes above, a test,
and fix of the typo you pointed out.
Regards,
Yugo Nagata
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA <nag...@sraoss.co.jp>
>
>
--
Yugo NAGATA <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 6c5c8afa6d..83fe01c33f 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -787,6 +787,19 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry id="pgbench-option-exit-on-abort">
+ <term><option>--exit-on-abort</option></term>
+ <listitem>
+ <para>
+ Exit immediately when any client is aborted due to some error. Without
+ this option, even when a client is aborted, other clients could continue
+ their run as specified by <option>-t</option> or <option>-T</option> option,
+ and <application>pgbench</application> will print an incomplete results
+ in this case.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="pgbench-option-log-prefix">
<term><option>--log-prefix=<replaceable>prefix</replaceable></option></term>
<listitem>
@@ -985,7 +998,8 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
benchmark such as initial connection failures also exit with status 1.
Errors during the run such as database errors or problems in the script
will result in exit status 2. In the latter case,
- <application>pgbench</application> will print partial results.
+ <application>pgbench</application> will print partial results if
+ <option>--exit-on-abort</option> option is not specified.
</para>
</refsect1>
@@ -2801,14 +2815,17 @@ statement latencies in milliseconds, failures and retries:
start a connection to the database server / the socket for connecting
the client to the database server has become invalid). In such cases
all clients of this thread stop while other threads continue to work.
+ However, <option>--exit-on-abort</option> is specified, all of the
+ threads stop immediately in this case.
</para>
</listitem>
<listitem>
<para>
Direct client errors. They lead to immediate exit from
<application>pgbench</application> with the corresponding error message
- only in the case of an internal <application>pgbench</application>
- error (which are supposed to never occur...). Otherwise in the worst
+ in the case of an internal <application>pgbench</application>
+ error (which are supposed to never occur...) or when
+ <option>--exit-on-abort</option> is specified . Otherwise in the worst
case they only lead to the abortion of the failed client while other
clients continue their run (but some client errors are handled without
an abortion of the client and reported separately, see below). Later in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..eca55d0230 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -765,6 +765,8 @@ static int64 total_weight = 0;
static bool verbose_errors = false; /* print verbose messages of all errors */
+static bool exit_on_abort = false; /* exit when any client is aborted */
+
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -911,6 +913,7 @@ usage(void)
" -T, --time=NUM duration of benchmark test in seconds\n"
" -v, --vacuum-all vacuum all four standard tables before tests\n"
" --aggregate-interval=NUM aggregate data over NUM seconds\n"
+ " --exit-on-abort exit when any client is aborted\n"
" --failures-detailed report the failures grouped by basic types\n"
" --log-prefix=PREFIX prefix for transaction time log file\n"
" (default: \"pgbench_log\")\n"
@@ -6612,6 +6615,7 @@ main(int argc, char **argv)
{"failures-detailed", no_argument, NULL, 13},
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
+ {"exit-on-abort", no_argument, NULL, 16},
{NULL, 0, NULL, 0}
};
@@ -6945,6 +6949,10 @@ main(int argc, char **argv)
benchmarking_option_set = true;
verbose_errors = true;
break;
+ case 16: /* exit-on-abort */
+ benchmarking_option_set = true;
+ exit_on_abort = true;
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7553,11 +7561,17 @@ threadRun(void *arg)
advanceConnectionState(thread, st, &aggs);
+ /*
+ * If --exit-on-abort is used, the program is going to exit
+ * when any client is aborted.
+ */
+ if (exit_on_abort && st->state == CSTATE_ABORTED)
+ goto done;
/*
* If advanceConnectionState changed client to finished state,
* that's one fewer client that remains.
*/
- if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
+ else if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
remains--;
}
@@ -7590,6 +7604,19 @@ threadRun(void *arg)
}
done:
+ if (exit_on_abort)
+ {
+ /* Abort if any client is not finished, meaning some error occurred. */
+ for (int i = 0; i < nstate; i++)
+ {
+ if (state[i].state != CSTATE_FINISHED)
+ {
+ pg_log_error("Run was aborted due to an error in thread %d", thread->tid);
+ exit(2);
+ }
+ }
+ }
+
disconnect_all(state, nstate);
if (thread->logfile)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 142f966300..0fe100232e 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -1475,6 +1475,30 @@ SELECT pg_advisory_unlock_all();
# Clean up
$node->safe_psql('postgres', 'DROP TABLE first_client_table, xy;');
+# Test --exit-on-abort
+$node->safe_psql('postgres',
+ 'CREATE TABLE counter(i int);'
+);
+
+$node->pgbench(
+ '-t 10 -c 4 -j 4 --exit-on-abort',
+ 2,
+ [],
+ [
+ qr{Run was aborted due to an error in thread}
+ ],
+ 'test --exit-on-abort',
+ {
+ '001_exit_on_abort' => q{
+update x set i = i+1 returning i \gset
+\if :i = 5
+\set y 1/0
+\endif
+}
+ });
+
+# Clean up
+$node->safe_psql('postgres', 'DROP TABLE counter;');
# done
$node->safe_psql('postgres', 'DROP TABLESPACE regress_pgbench_tap_1_ts');
