Hello Alvaro,
Thanks for the review and improvements.
Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e
Attached v5. I thought that separating the part that executes the
command was an obvious readability improvement.
Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
transitions to happen in doCustom's switch (st->state) and nowhere else,
which is defeated by creating the separate function.
Although it improves readability at one level, it does not help figuring
out what happens to states, which is my primary concern: The idea is that
reading doCustom is enough to build and check the automaton, which I had
to do repeatedly while reviewing Marina's patches.
Also the added doCustom comment, which announces this property becomes
false under the refactoring function:
All state changes are performed within this function called by
threadRun.
So I would suggest not to create this function.
Do we really use the word "move" to talk about state changes? It sounds
very odd to me. I would change that to "transition" -- would anybody
object to that? (Not changed in v5.)
Yep. I removed the goto:-)
On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
macro -- consider this:
if (foo)
INSTR_TIME_SET_CURRENT_LAZY(bar);
else
something_else();
Which "if" is the else now attached to? Now maybe the C standard has an
answer for that (I don't know what it is), but it's hard to read and
likely the compiler will complain anyway. I wrapped it in "do { }
while(0)" as is customary.
Indeed, good catch.
In the attached patched, I have I think included all your changes *but*
the separate function, for the reason discussed above, and removed the
"goto".
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 73d3de0677..82cbd20420 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -294,24 +294,32 @@ typedef enum
{
/*
* The client must first choose a script to execute. Once chosen, it can
- * either be throttled (state CSTATE_START_THROTTLE under --rate) or start
- * right away (state CSTATE_START_TX).
+ * either be throttled (state CSTATE_START_THROTTLE under --rate), start
+ * right away (state CSTATE_START_TX) or not start at all if the timer was
+ * exceeded (state CSTATE_FINISHED).
*/
CSTATE_CHOOSE_SCRIPT,
/*
* In CSTATE_START_THROTTLE state, we calculate when to begin the next
* transaction, and advance to CSTATE_THROTTLE. CSTATE_THROTTLE state
- * sleeps until that moment. (If throttling is not enabled, doCustom()
- * falls directly through from CSTATE_START_THROTTLE to CSTATE_START_TX.)
+ * sleeps until that moment.
+ *
+ * It may also detect that the next transaction would start beyond the end
+ * of run, and switch to CSTATE_FINISHED.
*/
CSTATE_START_THROTTLE,
CSTATE_THROTTLE,
/*
* CSTATE_START_TX performs start-of-transaction processing. Establishes
- * a new connection for the transaction, in --connect mode, and records
- * the transaction start time.
+ * a new connection for the transaction in --connect mode, records
+ * the transaction start time, and proceed to the first command.
+ *
+ * Note: once a script is started, it will either error or run till
+ * its end, where it may be interrupted. It is not interrupted while
+ * running, so pgbench --time is to be understood as tx are allowed to
+ * start in that time, and will finish when their work is completed.
*/
CSTATE_START_TX,
@@ -324,9 +332,6 @@ typedef enum
* and we enter the CSTATE_SLEEP state to wait for it to expire. Other
* meta-commands are executed immediately.
*
- * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
- * quickly skip commands that do not need any evaluation.
- *
* CSTATE_WAIT_RESULT waits until we get a result set back from the server
* for the current command.
*
@@ -334,19 +339,25 @@ typedef enum
*
* CSTATE_END_COMMAND records the end-of-command timestamp, increments the
* command counter, and loops back to CSTATE_START_COMMAND state.
+ *
+ * CSTATE_SKIP_COMMAND is used by conditional branches which are not
+ * executed. It quickly skip commands that do not need any evaluation.
+ * This state can move forward several commands, till there is something
+ * to do or the end of the script.
*/
CSTATE_START_COMMAND,
- CSTATE_SKIP_COMMAND,
CSTATE_WAIT_RESULT,
CSTATE_SLEEP,
CSTATE_END_COMMAND,
+ CSTATE_SKIP_COMMAND,
/*
- * CSTATE_END_TX performs end-of-transaction processing. Calculates
- * latency, and logs the transaction. In --connect mode, closes the
- * current connection. Chooses the next script to execute and starts over
- * in CSTATE_START_THROTTLE state, or enters CSTATE_FINISHED if we have no
- * more work to do.
+ * CSTATE_END_TX performs end-of-transaction processing. It calculates
+ * latency, and logs the transaction. In --connect mode, it closes the
+ * current connection.
+ *
+ * Then either starts over in CSTATE_CHOOSE_SCRIPT, or enters CSTATE_FINISHED
+ * if we have no more work to do.
*/
CSTATE_END_TX,
@@ -2821,16 +2832,12 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
/*
* Advance the state machine of a connection, if possible.
+ *
+ * All state changes are performed within this function called by threadRun.
*/
static void
doCustom(TState *thread, CState *st, StatsData *agg)
{
- PGresult *res;
- Command *command;
- instr_time now;
- bool end_tx_processed = false;
- int64 wait;
-
/*
* gettimeofday() isn't free, so we get the current timestamp lazily the
* first time it's needed, and reuse the same value throughout this
@@ -2839,37 +2846,44 @@ doCustom(TState *thread, CState *st, StatsData *agg)
* means "not set yet". Reset "now" when we execute shell commands or
* expressions, which might take a non-negligible amount of time, though.
*/
+ instr_time now;
INSTR_TIME_SET_ZERO(now);
/*
* Loop in the state machine, until we have to wait for a result from the
- * server (or have to sleep, for throttling or for \sleep).
+ * server or have to sleep for throttling or \sleep.
*
* Note: In the switch-statement below, 'break' will loop back here,
* meaning "continue in the state machine". Return is used to return to
- * the caller.
+ * the caller, giving the thread the opportunity to advance another client.
*/
for (;;)
{
+ PGresult *res;
+ Command *command;
+
switch (st->state)
{
/*
* Select transaction to run.
*/
case CSTATE_CHOOSE_SCRIPT:
-
st->use_file = chooseScript(thread);
if (debug)
fprintf(stderr, "client %d executing script \"%s\"\n", st->id,
sql_script[st->use_file].desc);
- if (throttle_delay > 0)
+ /* check stack consistency */
+ Assert(conditional_stack_empty(st->cstack));
+
+ if (timer_exceeded)
+ st->state = CSTATE_FINISHED;
+ else if (throttle_delay > 0)
st->state = CSTATE_START_THROTTLE;
else
st->state = CSTATE_START_TX;
- /* check consistency */
- Assert(conditional_stack_empty(st->cstack));
+
break;
/*
@@ -2887,21 +2901,10 @@ doCustom(TState *thread, CState *st, StatsData *agg)
* away.
*/
Assert(throttle_delay > 0);
- wait = getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
- thread->throttle_trigger += wait;
+ thread->throttle_trigger += getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
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
@@ -2913,20 +2916,20 @@ doCustom(TState *thread, CState *st, StatsData *agg)
{
int64 now_us;
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
now_us = INSTR_TIME_GET_MICROSEC(now);
+
while (thread->throttle_trigger < now_us - latency_limit &&
(nxacts <= 0 || st->cnt < nxacts))
{
processXactStats(thread, st, &now, true, agg);
/* next rendez-vous */
- wait = getPoissonRand(&thread->ts_throttle_rs,
- throttle_delay);
- thread->throttle_trigger += wait;
+ thread->throttle_trigger +=
+ getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
st->txn_scheduled = thread->throttle_trigger;
}
- /* stop client if -t exceeded */
+
+ /* stop client if -t was exceeded in the previous skip loop */
if (nxacts > 0 && st->cnt >= nxacts)
{
st->state = CSTATE_FINISHED;
@@ -2934,38 +2937,42 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
}
+ /*
+ * stop client if next transaction is beyond pgbench end of
+ * execution.
+ */
+ if (duration > 0 && st->txn_scheduled > end_time)
+ {
+ st->state = CSTATE_FINISHED;
+ break;
+ }
+
st->state = CSTATE_THROTTLE;
- if (debug)
- fprintf(stderr, "client %d throttling " INT64_FORMAT " us\n",
- st->id, wait);
break;
/*
* Wait until it's time to start next transaction.
*/
case CSTATE_THROTTLE:
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+
+ INSTR_TIME_SET_CURRENT_LAZY(now);
+
if (INSTR_TIME_GET_MICROSEC(now) < st->txn_scheduled)
- return; /* Still sleeping, nothing to do here */
+ return; /* still sleeping, nothing to do here */
- /* Else done sleeping, start the transaction */
- st->state = CSTATE_START_TX;
+ /* done sleeping, but do not start transaction if we are done */
+ st->state = timer_exceeded ? CSTATE_FINISHED : CSTATE_START_TX;
break;
/* Start new transaction */
case CSTATE_START_TX:
- /*
- * Establish connection on first call, or if is_connect is
- * true.
- */
+ /* establish connection if needed, i.e. under --connect */
if (st->con == NULL)
{
instr_time start;
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
start = now;
if ((st->con = doConnect()) == NULL)
{
@@ -2981,28 +2988,20 @@ doCustom(TState *thread, CState *st, StatsData *agg)
memset(st->prepared, 0, sizeof(st->prepared));
}
+ /* record transaction start time. */
+ INSTR_TIME_SET_CURRENT_LAZY(now);
+ st->txn_begin = now;
+
/*
- * Record transaction start time under logging, progress or
- * throttling.
+ * When not throttling, this is also the transaction's
+ * scheduled start time.
*/
- if (use_log || progress || throttle_delay || latency_limit ||
- per_script_stats)
- {
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
- st->txn_begin = now;
-
- /*
- * When not throttling, this is also the transaction's
- * scheduled start time.
- */
- if (!throttle_delay)
- st->txn_scheduled = INSTR_TIME_GET_MICROSEC(now);
- }
+ if (!throttle_delay)
+ st->txn_scheduled = INSTR_TIME_GET_MICROSEC(now);
/* Begin with the first command */
- st->command = 0;
st->state = CSTATE_START_COMMAND;
+ st->command = 0;
break;
/*
@@ -3021,17 +3020,11 @@ doCustom(TState *thread, CState *st, StatsData *agg)
break;
}
- /*
- * Record statement start time if per-command latencies are
- * requested
- */
- if (is_latencies)
- {
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
- st->stmt_begin = now;
- }
+ /* record statement start time. */
+ INSTR_TIME_SET_CURRENT_LAZY(now);
+ st->stmt_begin = now;
+ /* execute the command */
if (command->type == SQL_COMMAND)
{
if (!sendCommand(st, command))
@@ -3074,8 +3067,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
break;
}
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
+
st->sleep_until = INSTR_TIME_GET_MICROSEC(now) + usec;
st->state = CSTATE_SLEEP;
break;
@@ -3093,10 +3086,11 @@ doCustom(TState *thread, CState *st, StatsData *agg)
{
/*
* elif after executed block, skip eval and wait
- * for endif
+ * for endif. This is a shortcut.
*/
conditional_stack_poke(st->cstack, IFSTATE_IGNORED);
- goto move_to_end_command;
+ st->state = CSTATE_END_COMMAND;
+ break;
}
if (!evaluateExpr(thread, st, expr, &result))
@@ -3126,10 +3120,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
else /* elif */
{
- /*
- * we should get here only if the "elif"
- * needed evaluation
- */
+ /* we should get here only if the "elif" needed evaluation */
Assert(conditional_stack_peek(st->cstack) == IFSTATE_FALSE);
conditional_stack_poke(st->cstack, cond ? IFSTATE_TRUE : IFSTATE_FALSE);
}
@@ -3151,56 +3142,33 @@ doCustom(TState *thread, CState *st, StatsData *agg)
/* dead code if conditional check is ok */
Assert(false);
}
- goto move_to_end_command;
}
else if (command->meta == META_ENDIF)
{
Assert(!conditional_stack_empty(st->cstack));
conditional_stack_pop(st->cstack);
- goto move_to_end_command;
}
else if (command->meta == META_SETSHELL)
{
- bool ret = runShellCommand(st, argv[1], argv + 2, argc - 2);
-
- if (timer_exceeded) /* timeout */
- {
- st->state = CSTATE_FINISHED;
- break;
- }
- else if (!ret) /* on error */
+ if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
{
commandFailed(st, "setshell", "execution of meta-command failed");
st->state = CSTATE_ABORTED;
break;
}
- else
- {
- /* succeeded */
- }
+ /* else success */
}
else if (command->meta == META_SHELL)
{
- bool ret = runShellCommand(st, NULL, argv + 1, argc - 1);
-
- if (timer_exceeded) /* timeout */
- {
- st->state = CSTATE_FINISHED;
- break;
- }
- else if (!ret) /* on error */
+ if (!runShellCommand(st, NULL, argv + 1, argc - 1))
{
commandFailed(st, "shell", "execution of meta-command failed");
st->state = CSTATE_ABORTED;
break;
}
- else
- {
- /* succeeded */
- }
+ /* else success */
}
-
- move_to_end_command:
+ /* else nothing: all meta commands have been managed */
/*
* executing the expression or shell command might take a
@@ -3299,6 +3267,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
}
if (st->state != CSTATE_SKIP_COMMAND)
+ /* out of quick skip command loop */
break;
}
break;
@@ -3348,10 +3317,9 @@ doCustom(TState *thread, CState *st, StatsData *agg)
* instead of CSTATE_START_TX.
*/
case CSTATE_SLEEP:
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
if (INSTR_TIME_GET_MICROSEC(now) < st->sleep_until)
- return; /* Still sleeping, nothing to do here */
+ return; /* still sleeping, nothing to do here */
/* Else done sleeping. */
st->state = CSTATE_END_COMMAND;
break;
@@ -3366,17 +3334,13 @@ doCustom(TState *thread, CState *st, StatsData *agg)
* in thread-local data structure, if per-command latencies
* are requested.
*/
- if (is_latencies)
- {
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
- /* XXX could use a mutex here, but we choose not to */
- command = sql_script[st->use_file].commands[st->command];
- addToSimpleStats(&command->stats,
- INSTR_TIME_GET_DOUBLE(now) -
- INSTR_TIME_GET_DOUBLE(st->stmt_begin));
- }
+ /* XXX could use a mutex here, but we choose not to */
+ command = sql_script[st->use_file].commands[st->command];
+ addToSimpleStats(&command->stats,
+ INSTR_TIME_GET_DOUBLE(now) -
+ INSTR_TIME_GET_DOUBLE(st->stmt_begin));
/* Go ahead with next command, to be executed or skipped */
st->command++;
@@ -3385,19 +3349,15 @@ doCustom(TState *thread, CState *st, StatsData *agg)
break;
/*
- * End of transaction.
+ * End of transaction (end of script, really).
*/
case CSTATE_END_TX:
/* transaction finished: calculate latency and do log */
processXactStats(thread, st, &now, false, agg);
- /* conditional stack must be empty */
- if (!conditional_stack_empty(st->cstack))
- {
- fprintf(stderr, "end of script reached within a conditional, missing \\endif\n");
- exit(1);
- }
+ /* missing \endif... cannot happen if CheckConditional was okay */
+ Assert(conditional_stack_empty(st->cstack));
if (is_connect)
{
@@ -3411,26 +3371,17 @@ doCustom(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_FINISHED;
break;
}
+ else
+ {
+ /* next transaction */
+ st->state = CSTATE_CHOOSE_SCRIPT;
- /*
- * No transaction is underway anymore.
- */
- st->state = CSTATE_CHOOSE_SCRIPT;
-
- /*
- * If we paced through all commands in the script in this
- * loop, without returning to the caller even once, do it now.
- * This gives the thread a chance to process other
- * connections, and to do progress reporting. This can
- * currently only happen if the script consists entirely of
- * meta-commands.
- */
- if (end_tx_processed)
+ /*
+ * Ensure that we always return on this point, so as
+ * to avoid an infinite loop if the script only contains
+ * meta commands.
+ */
return;
- else
- {
- end_tx_processed = true;
- break;
}
/*
@@ -3544,8 +3495,7 @@ processXactStats(TState *thread, CState *st, instr_time *now,
if (detailed && !skipped)
{
- if (INSTR_TIME_IS_ZERO(*now))
- INSTR_TIME_SET_CURRENT(*now);
+ INSTR_TIME_SET_CURRENT_LAZY(*now);
/* compute latency & lag */
latency = INSTR_TIME_GET_MICROSEC(*now) - st->txn_scheduled;
@@ -5809,7 +5759,7 @@ threadRun(void *arg)
if (!is_connect)
{
- /* make connections to the database */
+ /* make connections to the database before starting */
for (i = 0; i < nstate; i++)
{
if ((state[i].con = doConnect()) == NULL)
@@ -5845,14 +5795,7 @@ threadRun(void *arg)
{
CState *st = &state[i];
- if (st->state == CSTATE_THROTTLE && timer_exceeded)
- {
- /* interrupt client that has not started a transaction */
- st->state = CSTATE_FINISHED;
- finishCon(st);
- remains--;
- }
- else if (st->state == CSTATE_SLEEP || st->state == CSTATE_THROTTLE)
+ if (st->state == CSTATE_SLEEP || st->state == CSTATE_THROTTLE)
{
/* a nap from the script, or under throttling */
int64 this_usec;
diff --git a/src/include/portability/instr_time.h b/src/include/portability/instr_time.h
index f968444671..db271c2822 100644
--- a/src/include/portability/instr_time.h
+++ b/src/include/portability/instr_time.h
@@ -20,6 +20,8 @@
*
* INSTR_TIME_SET_CURRENT(t) set t to current time
*
+ * INSTR_TIME_SET_CURRENT_LAZY(t) set t to current time if t is zero
+ *
* INSTR_TIME_ADD(x, y) x += y
*
* INSTR_TIME_SUBTRACT(x, y) x -= y
@@ -245,4 +247,12 @@ GetTimerFrequency(void)
#endif /* WIN32 */
+/* same macro on all platforms */
+
+#define INSTR_TIME_SET_CURRENT_LAZY(t) \
+ do { \
+ if (INSTR_TIME_IS_ZERO(t)) \
+ INSTR_TIME_SET_CURRENT(t); \
+ } while (0)
+
#endif /* INSTR_TIME_H */
