On 2018-Nov-20, Fabien COELHO wrote:

> 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.

I adopted your patch, then while going over it I decided to go with my
separation proposal anyway, and re-did it.

Looking at the state of the code before this patch, I totally understand
that you want to get away from the current state of affairs.  However, I
don't think my proposal makes matters worse; actually it makes them
better.  Please give the code a honest look and think whether the
separation of machine state advances is really worse with my proposal.

I also renamed some things.  doCustom() was a pretty bad name, as was
CSTATE_START_THROTTLE.

> 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.

I agree the state advances in threadRun were real messy.

Thanks for getting rid of the goto.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 73d3de0677..710130b022 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_PREPARE_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
+	 * In CSTATE_PREPARE_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_PREPARE_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,
 
@@ -567,7 +578,10 @@ static void setNullValue(PgBenchValue *pv);
 static void setBoolValue(PgBenchValue *pv, bool bval);
 static void setIntValue(PgBenchValue *pv, int64 ival);
 static void setDoubleValue(PgBenchValue *pv, double dval);
-static bool evaluateExpr(TState *, CState *, PgBenchExpr *, PgBenchValue *);
+static bool evaluateExpr(TState *thread, CState *st, PgBenchExpr *expr,
+			 PgBenchValue *retval);
+static instr_time doExecuteCommand(TState *thread, CState *st,
+				 instr_time now);
 static void doLog(TState *thread, CState *st,
 	  StatsData *agg, bool skipped, double latency, double lag);
 static void processXactStats(TState *thread, CState *st, instr_time *now,
@@ -2820,16 +2834,12 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
 }
 
 /*
- * Advance the state machine of a connection, if possible.
+ * Advance the state machine of a connection.
  */
 static void
-doCustom(TState *thread, CState *st, StatsData *agg)
+advanceConnectionState(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
@@ -2843,129 +2853,45 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 
 	/*
 	 * 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;
+
 		switch (st->state)
 		{
-				/*
-				 * Select transaction to run.
-				 */
+				/* Select transaction (script) to run.  */
 			case CSTATE_CHOOSE_SCRIPT:
-
 				st->use_file = chooseScript(thread);
+				Assert(conditional_stack_empty(st->cstack));
 
 				if (debug)
 					fprintf(stderr, "client %d executing script \"%s\"\n", st->id,
 							sql_script[st->use_file].desc);
 
-				if (throttle_delay > 0)
-					st->state = CSTATE_START_THROTTLE;
-				else
-					st->state = CSTATE_START_TX;
-				/* check consistency */
-				Assert(conditional_stack_empty(st->cstack));
+				/*
+				 * If time is over, we're done; otherwise, get ready to start
+				 * a new transaction, or to get throttled if that's requested.
+				 */
+				st->state = timer_exceeded ? CSTATE_FINISHED :
+					throttle_delay > 0 ? CSTATE_PREPARE_THROTTLE : CSTATE_START_TX;
 				break;
 
-				/*
-				 * Handle throttling once per transaction by sleeping.
-				 */
-			case CSTATE_START_THROTTLE:
-
-				/*
-				 * Generate a delay such that the series of delays will
-				 * approximate a Poisson distribution centered on the
-				 * throttle_delay time.
-				 *
-				 * If transactions are too slow or a given wait is shorter
-				 * than a transaction, the next transaction will start right
-				 * away.
-				 */
-				Assert(throttle_delay > 0);
-				wait = getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
-
-				thread->throttle_trigger += wait;
-				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
-				 * iterate till the next slot that isn't late yet.  But don't
-				 * iterate beyond the -t limit, if one is given.
-				 */
-				if (latency_limit)
-				{
-					int64		now_us;
-
-					if (INSTR_TIME_IS_ZERO(now))
-						INSTR_TIME_SET_CURRENT(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;
-						st->txn_scheduled = thread->throttle_trigger;
-					}
-					/* stop client if -t exceeded */
-					if (nxacts > 0 && st->cnt >= nxacts)
-					{
-						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);
-				if (INSTR_TIME_GET_MICROSEC(now) < st->txn_scheduled)
-					return;		/* Still sleeping, nothing to do here */
-
-				/* Else done sleeping, start the transaction */
-				st->state = CSTATE_START_TX;
-				break;
-
-				/* Start new transaction */
+				/* Start new transaction (script) */
 			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,235 +2907,126 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					memset(st->prepared, 0, sizeof(st->prepared));
 				}
 
-				/*
-				 * Record transaction start time under logging, progress or
-				 * throttling.
-				 */
-				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;
+				/* record transaction start time */
+				INSTR_TIME_SET_CURRENT_LAZY(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);
-				}
+				/*
+				 * When not throttling, this is also the transaction's
+				 * scheduled start time.
+				 */
+				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;
+
+				/*
+				 * Handle throttling once per transaction by sleeping.
+				 */
+			case CSTATE_PREPARE_THROTTLE:
+
+				/*
+				 * Generate a delay such that the series of delays will
+				 * approximate a Poisson distribution centered on the
+				 * throttle_delay time.
+				 *
+				 * If transactions are too slow or a given wait is shorter
+				 * than a transaction, the next transaction will start right
+				 * away.
+				 */
+				Assert(throttle_delay > 0);
+
+				thread->throttle_trigger +=
+					getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
+				st->txn_scheduled = thread->throttle_trigger;
+
+				/*
+				 * 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, skip this time slot and schedule
+				 * to continue running on the next slot that isn't late yet.
+				 * But don't iterate beyond the -t limit, if one is given.
+				 */
+				if (latency_limit)
+				{
+					int64		now_us;
+
+					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 */
+						thread->throttle_trigger +=
+							getPoissonRand(&thread->ts_throttle_rs, throttle_delay);
+						st->txn_scheduled = thread->throttle_trigger;
+					}
+
+					/*
+					 * stop client if -t was exceeded in the previous skip
+					 * loop
+					 */
+					if (nxacts > 0 && st->cnt >= nxacts)
+					{
+						st->state = CSTATE_FINISHED;
+						break;
+					}
+				}
+
+				/*
+				 * stop client if next transaction is beyond pgbench end of
+				 * execution; otherwise, throttle it.
+				 */
+				st->state = end_time > 0 && st->txn_scheduled > end_time ?
+					CSTATE_FINISHED : CSTATE_THROTTLE;
+				break;
+
+				/*
+				 * Wait until it's time to start next transaction.
+				 */
+			case CSTATE_THROTTLE:
+				INSTR_TIME_SET_CURRENT_LAZY(now);
+
+				if (INSTR_TIME_GET_MICROSEC(now) < st->txn_scheduled)
+					return;		/* still sleeping, nothing to do here */
+
+				/* done sleeping, but don't start transaction if we're done */
+				st->state = timer_exceeded ? CSTATE_FINISHED : CSTATE_START_TX;
 				break;
 
 				/*
 				 * Send a command to server (or execute a meta-command)
 				 */
 			case CSTATE_START_COMMAND:
-				command = sql_script[st->use_file].commands[st->command];
-
-				/*
-				 * If we reached the end of the script, move to end-of-xact
-				 * processing.
-				 */
-				if (command == NULL)
+				/* Transition to script end processing if done */
+				if (sql_script[st->use_file].commands[st->command] == NULL)
 				{
 					st->state = CSTATE_END_TX;
 					break;
 				}
 
-				/*
-				 * Record statement start time if per-command latencies are
-				 * requested
-				 */
+				/* record begin time of next command, and initiate it */
 				if (is_latencies)
 				{
-					if (INSTR_TIME_IS_ZERO(now))
-						INSTR_TIME_SET_CURRENT(now);
+					INSTR_TIME_SET_CURRENT_LAZY(now);
 					st->stmt_begin = now;
 				}
+				now = doExecuteCommand(thread, st, now);
 
-				if (command->type == SQL_COMMAND)
-				{
-					if (!sendCommand(st, command))
-					{
-						commandFailed(st, "SQL", "SQL command send failed");
-						st->state = CSTATE_ABORTED;
-					}
-					else
-						st->state = CSTATE_WAIT_RESULT;
-				}
-				else if (command->type == META_COMMAND)
-				{
-					int			argc = command->argc,
-								i;
-					char	  **argv = command->argv;
-
-					if (debug)
-					{
-						fprintf(stderr, "client %d executing \\%s", st->id, argv[0]);
-						for (i = 1; i < argc; i++)
-							fprintf(stderr, " %s", argv[i]);
-						fprintf(stderr, "\n");
-					}
-
-					if (command->meta == META_SLEEP)
-					{
-						/*
-						 * A \sleep doesn't execute anything, we just get the
-						 * delay from the argument, and enter the CSTATE_SLEEP
-						 * state.  (The per-command latency will be recorded
-						 * in CSTATE_SLEEP state, not here, after the delay
-						 * has elapsed.)
-						 */
-						int			usec;
-
-						if (!evaluateSleep(st, argc, argv, &usec))
-						{
-							commandFailed(st, "sleep", "execution of meta-command failed");
-							st->state = CSTATE_ABORTED;
-							break;
-						}
-
-						if (INSTR_TIME_IS_ZERO(now))
-							INSTR_TIME_SET_CURRENT(now);
-						st->sleep_until = INSTR_TIME_GET_MICROSEC(now) + usec;
-						st->state = CSTATE_SLEEP;
-						break;
-					}
-					else if (command->meta == META_SET ||
-							 command->meta == META_IF ||
-							 command->meta == META_ELIF)
-					{
-						/* backslash commands with an expression to evaluate */
-						PgBenchExpr *expr = command->expr;
-						PgBenchValue result;
-
-						if (command->meta == META_ELIF &&
-							conditional_stack_peek(st->cstack) == IFSTATE_TRUE)
-						{
-							/*
-							 * elif after executed block, skip eval and wait
-							 * for endif
-							 */
-							conditional_stack_poke(st->cstack, IFSTATE_IGNORED);
-							goto move_to_end_command;
-						}
-
-						if (!evaluateExpr(thread, st, expr, &result))
-						{
-							commandFailed(st, argv[0], "evaluation of meta-command failed");
-							st->state = CSTATE_ABORTED;
-							break;
-						}
-
-						if (command->meta == META_SET)
-						{
-							if (!putVariableValue(st, argv[0], argv[1], &result))
-							{
-								commandFailed(st, "set", "assignment of meta-command failed");
-								st->state = CSTATE_ABORTED;
-								break;
-							}
-						}
-						else	/* if and elif evaluated cases */
-						{
-							bool		cond = valueTruth(&result);
-
-							/* execute or not depending on evaluated condition */
-							if (command->meta == META_IF)
-							{
-								conditional_stack_push(st->cstack, cond ? IFSTATE_TRUE : IFSTATE_FALSE);
-							}
-							else	/* elif */
-							{
-								/*
-								 * 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);
-							}
-						}
-					}
-					else if (command->meta == META_ELSE)
-					{
-						switch (conditional_stack_peek(st->cstack))
-						{
-							case IFSTATE_TRUE:
-								conditional_stack_poke(st->cstack, IFSTATE_ELSE_FALSE);
-								break;
-							case IFSTATE_FALSE: /* inconsistent if active */
-							case IFSTATE_IGNORED:	/* inconsistent if active */
-							case IFSTATE_NONE:	/* else without if */
-							case IFSTATE_ELSE_TRUE: /* else after else */
-							case IFSTATE_ELSE_FALSE:	/* else after else */
-							default:
-								/* 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 */
-						{
-							commandFailed(st, "setshell", "execution of meta-command failed");
-							st->state = CSTATE_ABORTED;
-							break;
-						}
-						else
-						{
-							/* succeeded */
-						}
-					}
-					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 */
-						{
-							commandFailed(st, "shell", "execution of meta-command failed");
-							st->state = CSTATE_ABORTED;
-							break;
-						}
-						else
-						{
-							/* succeeded */
-						}
-					}
-
-			move_to_end_command:
-
-					/*
-					 * executing the expression or shell command might take a
-					 * non-negligible amount of time, so reset 'now'
-					 */
-					INSTR_TIME_SET_ZERO(now);
-
-					st->state = CSTATE_END_COMMAND;
-				}
+				/*
+				 * We're now waiting for an SQL command to complete, or
+				 * finished processing a metacommand, or need to sleep, or
+				 * something bad happened.
+				 */
+				Assert(st->state == CSTATE_WAIT_RESULT ||
+					   st->state == CSTATE_END_COMMAND ||
+					   st->state == CSTATE_SLEEP ||
+					   st->state == CSTATE_ABORTED);
 				break;
 
 				/*
@@ -3220,6 +3037,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 				/* quickly skip commands until something to do... */
 				while (true)
 				{
+					Command    *command;
+
 					command = sql_script[st->use_file].commands[st->command];
 
 					/* cannot reach end of script in that state */
@@ -3299,6 +3118,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					}
 
 					if (st->state != CSTATE_SKIP_COMMAND)
+						/* out of quick skip command loop */
 						break;
 				}
 				break;
@@ -3307,11 +3127,11 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 				 * Wait for the current SQL command to complete
 				 */
 			case CSTATE_WAIT_RESULT:
-				command = sql_script[st->use_file].commands[st->command];
 				if (debug)
 					fprintf(stderr, "client %d receiving\n", st->id);
 				if (!PQconsumeInput(st->con))
-				{				/* there's something wrong */
+				{
+					/* there's something wrong */
 					commandFailed(st, "SQL", "perhaps the backend died while processing");
 					st->state = CSTATE_ABORTED;
 					break;
@@ -3319,9 +3139,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 				if (PQisBusy(st->con))
 					return;		/* don't have the whole result yet */
 
-				/*
-				 * Read and discard the query result;
-				 */
+				/* Read and discard the query result */
 				res = PQgetResult(st->con);
 				switch (PQresultStatus(res))
 				{
@@ -3348,10 +3166,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;
@@ -3368,11 +3185,11 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 				 */
 				if (is_latencies)
 				{
-					if (INSTR_TIME_IS_ZERO(now))
-						INSTR_TIME_SET_CURRENT(now);
+					Command    *command = sql_script[st->use_file].commands[st->command];
+
+					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));
@@ -3385,19 +3202,18 @@ 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 +3227,17 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 					st->state = CSTATE_FINISHED;
 					break;
 				}
-
-				/*
-				 * 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)
-					return;
 				else
 				{
-					end_tx_processed = true;
-					break;
+					/* next transaction (script) */
+					st->state = CSTATE_CHOOSE_SCRIPT;
+
+					/*
+					 * Ensure that we always return on this point, so as to
+					 * avoid an infinite loop if the script only contains meta
+					 * commands.
+					 */
+					return;
 				}
 
 				/*
@@ -3445,6 +3252,182 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 }
 
 /*
+ * Subroutine for advanceConnectionState -- execute or initiate the current
+ * command, and transition to next state appropriately.
+ *
+ * Returns an updated timestamp from 'now', used to update 'now' at callsite.
+ */
+static instr_time
+doExecuteCommand(TState *thread, CState *st, instr_time now)
+{
+	Command    *command = sql_script[st->use_file].commands[st->command];
+
+	/* execute the command */
+	if (command->type == SQL_COMMAND)
+	{
+		if (!sendCommand(st, command))
+		{
+			commandFailed(st, "SQL", "SQL command send failed");
+			st->state = CSTATE_ABORTED;
+		}
+		else
+			st->state = CSTATE_WAIT_RESULT;
+	}
+	else if (command->type == META_COMMAND)
+	{
+		int			argc = command->argc;
+		char	  **argv = command->argv;
+
+		if (debug)
+		{
+			fprintf(stderr, "client %d executing \\%s",
+					st->id, argv[0]);
+			for (int i = 1; i < argc; i++)
+				fprintf(stderr, " %s", argv[i]);
+			fprintf(stderr, "\n");
+		}
+
+		if (command->meta == META_SLEEP)
+		{
+			int			usec;
+
+			/*
+			 * A \sleep doesn't execute anything, we just get the delay from
+			 * the argument, and enter the CSTATE_SLEEP state.  (The
+			 * per-command latency will be recorded in CSTATE_SLEEP state, not
+			 * here, after the delay has elapsed.)
+			 */
+			if (!evaluateSleep(st, argc, argv, &usec))
+			{
+				commandFailed(st, "sleep", "execution of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+
+			INSTR_TIME_SET_CURRENT_LAZY(now);
+
+			st->sleep_until = INSTR_TIME_GET_MICROSEC(now) + usec;
+			st->state = CSTATE_SLEEP;
+			return now;
+		}
+		else if (command->meta == META_SET)
+		{
+			PgBenchExpr *expr = command->expr;
+			PgBenchValue result;
+
+			if (!evaluateExpr(thread, st, expr, &result))
+			{
+				commandFailed(st, argv[0], "evaluation of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+
+			if (!putVariableValue(st, argv[0], argv[1], &result))
+			{
+				commandFailed(st, "set", "assignment of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+		}
+		else if (command->meta == META_IF)
+		{
+			/* backslash commands with an expression to evaluate */
+			PgBenchExpr *expr = command->expr;
+			PgBenchValue result;
+			bool		cond;
+
+			if (!evaluateExpr(thread, st, expr, &result))
+			{
+				commandFailed(st, argv[0], "evaluation of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+
+			cond = valueTruth(&result);
+			conditional_stack_push(st->cstack, cond ? IFSTATE_TRUE : IFSTATE_FALSE);
+		}
+		else if (command->meta == META_ELIF)
+		{
+			/* backslash commands with an expression to evaluate */
+			PgBenchExpr *expr = command->expr;
+			PgBenchValue result;
+			bool		cond;
+
+			if (conditional_stack_peek(st->cstack) == IFSTATE_TRUE)
+			{
+				/*
+				 * elif after executed block, skip eval and wait for endif.
+				 */
+				conditional_stack_poke(st->cstack, IFSTATE_IGNORED);
+				st->state = CSTATE_END_COMMAND;
+				return now;
+			}
+
+			if (!evaluateExpr(thread, st, expr, &result))
+			{
+				commandFailed(st, argv[0], "evaluation of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+
+			cond = valueTruth(&result);
+			Assert(conditional_stack_peek(st->cstack) == IFSTATE_FALSE);
+			conditional_stack_poke(st->cstack, cond ? IFSTATE_TRUE : IFSTATE_FALSE);
+		}
+		else if (command->meta == META_ELSE)
+		{
+			switch (conditional_stack_peek(st->cstack))
+			{
+				case IFSTATE_TRUE:
+					conditional_stack_poke(st->cstack, IFSTATE_ELSE_FALSE);
+					break;
+				case IFSTATE_FALSE: /* inconsistent if active */
+				case IFSTATE_IGNORED:	/* inconsistent if active */
+				case IFSTATE_NONE:	/* else without if */
+				case IFSTATE_ELSE_TRUE: /* else after else */
+				case IFSTATE_ELSE_FALSE:	/* else after else */
+				default:
+					/* dead code if conditional check is ok */
+					Assert(false);
+			}
+		}
+		else if (command->meta == META_ENDIF)
+		{
+			Assert(!conditional_stack_empty(st->cstack));
+			conditional_stack_pop(st->cstack);
+		}
+		else if (command->meta == META_SETSHELL)
+		{
+			if (!runShellCommand(st, argv[1], argv + 2, argc - 2))
+			{
+				commandFailed(st, "setshell", "execution of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+		}
+		else if (command->meta == META_SHELL)
+		{
+			if (!runShellCommand(st, NULL, argv + 1, argc - 1))
+			{
+				commandFailed(st, "shell", "execution of meta-command failed");
+				st->state = CSTATE_ABORTED;
+				return now;
+			}
+		}
+
+		/*
+		 * executing the expression or shell command might have taken a
+		 * non-negligible amount of time, so reset 'now'
+		 */
+		INSTR_TIME_SET_ZERO(now);
+
+		st->state = CSTATE_END_COMMAND;
+	}
+
+	return now;
+}
+
+/*
  * Print log entry after completing one transaction.
  *
  * We print Unix-epoch timestamps in the log, so that entries can be
@@ -3544,8 +3527,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 +5791,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 +5827,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;
@@ -5971,7 +5946,7 @@ threadRun(void *arg)
 
 			if (st->state == CSTATE_WAIT_RESULT)
 			{
-				/* don't call doCustom unless data is available */
+				/* don't call advanceConnectionState unless data is available */
 				int			sock = PQsocket(st->con);
 
 				if (sock < 0)
@@ -5991,9 +5966,12 @@ threadRun(void *arg)
 				continue;
 			}
 
-			doCustom(thread, st, &aggs);
+			advanceConnectionState(thread, st, &aggs);
 
-			/* If doCustom changed client to finished state, reduce remains */
+			/*
+			 * If advanceConnectionState changed client to finished state,
+			 * that's one less client that remains.
+			 */
 			if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
 				remains--;
 		}
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 */

Reply via email to