Hello Fabien,
On Thu, 17 Jun 2021 10:37:05 +0200 (CEST)
Fabien COELHO <[email protected]> wrote:
> > ). Is this acceptable for you?
>
> I disagree on two counts:
>
> First, thread[0] should not appear.
>
> Second, currently the *only* function to change the client state is
> advanceConnectionState, so it can be checked there and any bug is only
> there. We had issues before when several functions where doing updates,
> and it was a mess to understand what was going on. I really want that it
> stays that way, so I disagree with setting the state to ABORTED from
> threadRun. Moreover I do not see that it brings a feature, so ISTM that it
> is not an actual issue not to do it?
Ok. I gave up to change the state in threadRun. Instead, I changed the
condition at the end of bench, which enables to report abortion due to
socket errors.
+@@ -6480,7 +6490,7 @@ main(int argc, char **argv)
+ #endif /*
ENABLE_THREAD_SAFETY */
+
+ for (int j = 0; j < thread->nstate; j++)
+- if (thread->state[j].state == CSTATE_ABORTED)
++ if (thread->state[j].state != CSTATE_FINISHED)
+ exit_code = 2;
+
+ /* aggregate thread level stats */
Does this make sense?
--
Yugo NAGATA <[email protected]>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..901f25aab7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3171,6 +3171,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
if ((st->con = doConnect()) == NULL)
{
+ /* as the bench is already running, we do not abort */
pg_log_error("client %d aborted while establishing connection", st->id);
st->state = CSTATE_ABORTED;
break;
@@ -4405,7 +4406,10 @@ runInitSteps(const char *initialize_steps)
initPQExpBuffer(&stats);
if ((con = doConnect()) == NULL)
+ {
+ pg_log_fatal("connection for initialization failed");
exit(1);
+ }
setup_cancel_handler(NULL);
SetCancelConn(con);
@@ -6332,7 +6336,10 @@ main(int argc, char **argv)
/* opening connection... */
con = doConnect();
if (con == NULL)
+ {
+ pg_log_fatal("setup connection failed");
exit(1);
+ }
pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
PQhost(con), PQport(con), nclients,
@@ -6437,7 +6444,10 @@ main(int argc, char **argv)
errno = THREAD_BARRIER_INIT(&barrier, nthreads);
if (errno != 0)
+ {
pg_log_fatal("could not initialize barrier: %m");
+ exit(1);
+ }
#ifdef ENABLE_THREAD_SAFETY
/* start all threads but thread 0 which is executed directly later */
@@ -6480,7 +6490,7 @@ main(int argc, char **argv)
#endif /* ENABLE_THREAD_SAFETY */
for (int j = 0; j < thread->nstate; j++)
- if (thread->state[j].state == CSTATE_ABORTED)
+ if (thread->state[j].state != CSTATE_FINISHED)
exit_code = 2;
/* aggregate thread level stats */
@@ -6548,7 +6558,7 @@ threadRun(void *arg)
if (thread->logfile == NULL)
{
pg_log_fatal("could not open logfile \"%s\": %m", logpath);
- goto done;
+ exit(1);
}
}
@@ -6572,16 +6582,10 @@ threadRun(void *arg)
{
if ((state[i].con = doConnect()) == NULL)
{
- /*
- * On connection failure, we meet the barrier here in place of
- * GO before proceeding to the "done" path which will cleanup,
- * so as to avoid locking the process.
- *
- * It is unclear whether it is worth doing anything rather
- * than coldly exiting with an error message.
- */
- THREAD_BARRIER_WAIT(&barrier);
- goto done;
+ /* coldly abort on connection failure */
+ pg_log_fatal("cannot create connection for thread %d client %d",
+ thread->tid, i);
+ exit(1);
}
}
@@ -6707,7 +6711,7 @@ threadRun(void *arg)
continue;
}
/* must be something wrong */
- pg_log_fatal("%s() failed: %m", SOCKET_WAIT_METHOD);
+ pg_log_error("%s() failed: %m", SOCKET_WAIT_METHOD);
goto done;
}
}
