On 18/07/18 23:29, Fabien COELHO wrote:
Hmm. How about we just remove this special case from doCustom():
case CSTATE_START_THROTTLE:
// ...
if (duration > 0 && st->txn_scheduled > end_time)
{
st->state = CSTATE_FINISHED;
break;
}
That way, we let the client go into CSTATE_THROTTLE state, even though
we know that the timer will run out before we reach txn_scheduled.
Then it will work the way we want, right? One small difference is that
then the clients will keep the connections open longer, until the
timer expires, but I think that's reasonable. Less surprising than the
current behavior, even.
Hmmm... in this instance, and if this test is removed, ISTM that it can
start the transaction, re-establishing a connection under --connect, and
the transaction will run to its end even if it is beyond the expected end
of run. So removing this test does not seem desirable.
Can you elaborate? I don't think that's how it works. In threadRun(), we have
this:
The state path I want to avoid is, without getting out of doCustom, is:
CHOOSE_SCRIPT:
-> START_THROTTLE (i.e. under -R)
START_THROTTLE:
update txn_schedule, which happen to be after end_time,
i.e. the transaction is scheduled after the expected end of the run.
but if the condition is removed, then proceed directly to
-> THROTTLE
THROTTLE:
if now has passed txn_schedule (we are late), no return, proceed
directly to -> START_TX
START_TX:
-> START_COMMAND
START_COMMAND: executed... & "return" on SQL, but it is too late
for stopping the tx now, it has started.
Maybe I missed something, but it looks to me that we can get in the
situation where a transaction scheduled beyond the end of run is started
anyway if it happens that it was a little late.
>
[... threadRun ...]
As soon as the -T timer is exceeded, the above code will close all
connections that are in CSTATE_THROTTLE state.
So threadRun() would not have the opportunity to stop the scheduled
transaction, even if beyond the end of run, because it would not have got
out of doCustom, in the case I outlined above.
I see. Instead of moving to FINISHED state, then, we could stay in
THROTTLE state, and 'return' out of doCustom(), to give the code in
threadRun() a chance to detect that the timer is up. Something like the
attached. (I moved the check after the check for latency_limit, because
that code updates txn_scheduled. Seems more like a more correct place,
although that's as a separate issue.)
- Heikki
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 41b756c089..4662013b44 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2751,16 +2751,6 @@ doCustom(TState *thread, CState *st, StatsData *agg)
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)
- {
- st->state = CSTATE_FINISHED;
- break;
- }
-
- /*
* If --latency-limit is used, and this slot is already late
* so that the transaction will miss the latency limit even if
* it completed immediately, we skip this time slot and
@@ -2791,6 +2781,19 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
}
+ /*
+ * If next transaction is scheduled beyond pgbench end of
+ * execution, don't start a new transaction. We used to move
+ * straight to to CSTATE_FINISHED here, but it is pretty
+ * surprising if the test finished before the specified
+ * duration is up, even if the clients have nothing left to
+ * do. (In particular, it caused a regression test to fail,
+ * which tested that "pgbench -T 2" doesn't return before
+ * two seconds have passed.)
+ */
+ if (duration > 0 && st->txn_scheduled > end_time)
+ return;
+
st->state = CSTATE_THROTTLE;
if (debug)
fprintf(stderr, "client %d throttling " INT64_FORMAT " us\n",
