Hello Tom,
Ok. Here is an updated version, with a better suffix and a simplified
comment.
Doesn't this break the handling of latency calculations, or at least make
the results completely different for the last metacommand than what they
would be for a non-last command? It looks like it needs to loop back so
that the latency calculation is completed for the metacommand before it
can exit. Seems to me it would probably make more sense to fall out at
the end of the "transaction finished" if-block, around line 1923 in HEAD.
Indeed, it would trouble a little bit the stats computation by delaying
the recording of the end of statement & transaction.
However line 1923 is a shortcut for ending pgbench, but at the end of a
transaction more stuff must be done, eg choosing the next script and
reconnecting, before exiting. The solution is more contrived.
The attached patch provides a solution which ensures the return in the
right condition and after the stat collection. The code structure requires
another ugly boolean to proceed so as to preserve doing the reconnection
between the decision that the return must be done and the place where it
can be done, after reconnecting.
(The code structure in here seems like a complete mess to me, but probably
now is not the time to refactor it.)
I fully agree that the code structure is a total mess:-( Maybe I'll try to
submit a simpler one some day.
Basically the doCustom function is not resilient, you cannot exit from
anywhere and hope that re-entring would achieve a consistent behavior.
While reading the code to find a better place for a return, I noted some
possible inconsistencies in recording stats, which are noted as comments
in the attached patch.
Calling chooseScript is done both from outside for initialization and from
inside doCustom, where it could be done once and more clearly in doCustom.
Boolean listen is not reset because the script is expected to execute
directly the start of the next statement. I succeeded in convincing myself
that it actually works, but it is unobvious to spot why. I think that a
simpler pattern would be welcome. Also, some other things (eg prepared)
are not reset in all cases, not sure why.
The goto should probably be replaced by a while.
...
--
Fabien.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 87fb006..8c5df14 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1766,7 +1766,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
{
PGresult *res;
Command **commands;
- bool trans_needs_throttle = false;
+ bool trans_needs_throttle = false,
+ return_before_next_trans = false;
instr_time now;
/*
@@ -1849,6 +1850,8 @@ top:
if (st->listen)
{ /* are we receiver? */
+ bool listened_a_meta = commands[st->state]->type == META_COMMAND;
+
if (commands[st->state]->type == SQL_COMMAND)
{
if (debug)
@@ -1892,6 +1895,7 @@ top:
/*
* Read and discard the query result; note this is not included in
* the statement latency numbers.
+ * Should this be done before recording the statement stats?
*/
res = PQgetResult(st->con);
switch (PQresultStatus(res))
@@ -1913,6 +1917,7 @@ top:
{
if (is_connect)
{
+ /* Should transaction stats recorded above count this time? */
PQfinish(st->con);
st->con = NULL;
}
@@ -1942,12 +1947,17 @@ top:
* listen back to true.
*/
st->listen = false;
+
+ if (listened_a_meta)
+ return_before_next_trans = true;
+
trans_needs_throttle = (throttle_delay > 0);
}
}
if (st->con == NULL)
{
+ /* Why is connection time is out of transaction time stats? */
instr_time start,
end;
@@ -1969,6 +1979,10 @@ top:
memset(st->prepared, 0, sizeof(st->prepared));
}
+ /* ensure that meta-only scripts sometimes return */
+ if (return_before_next_trans)
+ return true;
+
/*
* This ensures that a throttling delay is inserted before proceeding with
* sql commands, after the first transaction. The first transaction
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
