On 08-09-2018 16:03, Fabien COELHO wrote:
Hello Marina,
v11-0003-Pgbench-errors-and-serialization-deadlock-retrie.patch
- the main patch for handling client errors and repetition of
transactions with serialization/deadlock failures (see the detailed
description in the file).
About patch v11-3.
Patch applies cleanly on top of the other two. Compiles, global and
local
"make check" are ok.
:-)
* Features
As far as the actual retry feature is concerned, I'd say we are nearly
there. However I have an issue with changing the behavior on meta
command and other sql errors, which I find not desirable.
When a meta-command fails, before the patch the command is aborted and
there is a convenient error message:
sh> pgbench -T 10 -f bad-meta.sql
bad-meta.sql:1: unexpected function name (false) in command "set"
[...]
\set i false + 1 [...]
After the patch it is simply counted, pgbench loops on the same error
till the time is completed, and there are no clue about the actual
issue:
sh> pgbench -T 10 -f bad-meta.sql
starting vacuum...end.
transaction type: bad-meta.sql
duration: 10 s
number of transactions actually processed: 0
number of failures: 27993953 (100.000%)
...
Same thing about SQL errors, an immediate abort...
sh> pgbench -T 10 -f bad-sql.sql
starting vacuum...end.
client 0 aborted in command 0 of script 0; ERROR: syntax error at or
near ";"
LINE 1: SELECT 1 + ;
... is turned into counting without aborting nor error messages, so
that there is no clue that the user was asking for something bad.
sh> pgbench -T 10 -f bad-sql.sql
starting vacuum...end.
transaction type: bad-sql.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
duration: 10 s
number of transactions actually processed: 0
number of failures: 274617 (100.000%)
# no clue that there was a syntax error in the script
I do not think that these changes of behavior are desirable. Meta
command and
miscellaneous SQL errors should result in immediatly aborting the whole
run,
because the client test code itself could not run correctly or the SQL
sent
was somehow wrong, which is also the client's fault, and the server
performance bench does not make much sense in such conditions.
ISTM that the focus of this patch should only be to handle some server
runtime errors that can be retryed, but not to change pgbench behavior
on other kind of errors. If these are to be changed, ISTM that it
would be a distinct patch and would require some discussion, and
possibly an option to enable it or not if some use case emerge. AFA
this patch is concerned, I'd suggest to let that out.
...
The following remarks are linked to the change of behavior discussed
above:
makeVariableValue error message is not for debug, but must be kept in
all
cases, and the false returned must result in an immediate abort. Same
thing about
lookupCreateVariable, an invalid name is a user error which warrants
an immediate
abort. Same thing again about coerce* functions or evalStandardFunc...
Basically, most/all added "debug_level >= DEBUG_ERRORS" are not
desirable.
Hmm, but we can say the same for serialization or deadlock errors that
were not retried (the client test code itself could not run correctly or
the SQL sent was somehow wrong, which is also the client's fault), can't
we? Why not handle client errors that can occur (but they may also not
occur) the same way? (For example, always abort the client, or
conversely do not make aborts in these cases.) Here's an example of such
error:
starting vacuum...end.
transaction type: pgbench_rare_sql_error.sql
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
number of transactions per client: 250
number of transactions actually processed: 2500/2500
maximum number of tries: 1
latency average = 0.375 ms
tps = 26695.292848 (including connections establishing)
tps = 27489.678525 (excluding connections establishing)
statement latencies in milliseconds and failures:
0.001 0 \set divider random(-1000, 1000)
0.245 0 SELECT 1 / :divider;
starting vacuum...end.
client 5 got an error in command 1 (SQL) of script 0; ERROR: division
by zero
client 0 got an error in command 1 (SQL) of script 0; ERROR: division
by zero
client 7 got an error in command 1 (SQL) of script 0; ERROR: division
by zero
transaction type: pgbench_rare_sql_error.sql
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
number of transactions per client: 250
number of transactions actually processed: 2497/2500
number of failures: 3 (0.120%)
number of serialization failures: 0 (0.000%)
number of deadlock failures: 0 (0.000%)
number of other SQL failures: 3 (0.120%)
maximum number of tries: 1
latency average = 0.579 ms (including failures)
tps = 17240.662547 (including connections establishing)
tps = 17862.090137 (excluding connections establishing)
statement latencies in milliseconds and failures:
0.001 0 \set divider random(-1000, 1000)
0.338 3 SELECT 1 / :divider;
Maybe we can limit the number of failures in one statement, and abort
the client if this limit is exceeded?...
To get a clue about the actual issue you can use the options
--failures-detailed (to find out out whether this is a serialization
failure / deadlock failure / other SQL failure / meta command failure)
and/or --print-errors (to get the complete error message).
Doc says "you cannot use an infinite number of retries without
latency-limit..."
Why should this be forbidden? At least if -T timeout takes precedent
and
shortens the execution, ISTM that there could be good reason to test
that.
Maybe it could be blocked only under -t if this would lead to an
non-ending
run.
...
* Comments
"There're different types..." -> "There are different types..."
"after the errors and"... -> "after errors and"...
"the default value of max_tries is set to 1" -> "the default value
of max_tries is 1"
"We cannot retry the transaction" -> "We cannot retry a transaction"
"may ultimately succeed or get a failure," -> "may ultimately succeed
or fail,"
...
* Documentation:
Some suggestions which may be improvements, although I'm not a native
English
speaker.
ISTM that there are too many "the":
- "turns on the option ..." -> "turns on option ..."
- "When the option ..." -> "When option ..."
- "By default the option ..." -> "By default option ..."
- "only if the option ..." -> "only if option ..."
- "combined with the option ..." -> "combined with option ..."
- "without the option ..." -> "without option ..."
- "is the sum of all the retries" -> "is the sum of all retries"
"infinite" -> "unlimited"
"not retried at all" -> "not retried" (maybe several times).
"messages of all errors" -> "messages about all errors".
"It is assumed that the scripts used do not contain" ->
"It is assumed that pgbench scripts do not contain"
Thank you, I'll fix this.
If you use the option --latency-limit, the time of tries will be limited
regardless of the use of the option -t. Therefore ISTM that an unlimited
number of tries can be used only if the time of tries is limited by the
options -T and/or -L.
As "--print-errors" is really for debug, maybe it could be named
"--debug-errors".
Ok!
I'm not sure that having "--debug" implying this option
is useful: As there are two distinct options, the user may be allowed
to trigger one or the other as they wish?
I'm not sure that the main debugging output will give a good clue of
what's happened without full messages about errors, retries and
failures...
* Code
<...>
sendRollback(): I'd suggest to simplify. The prepare/extended statement
stuff is
really about the transaction script, not dealing with errors, esp as
there is no
significant advantage in preparing a "ROLLBACK" statement which is
short and has
no parameters. I'd suggest to remove this function and just issue
PQsendQuery("ROLLBACK;") in all cases.
Ok!
In copyVariables, I'd simplify
+ if (source_var->svalue == NULL)
+ dest_var->svalue = NULL;
+ else
+ dest_var->svalue = pg_strdup(source_var->svalue);
as:
dest_var->value = (source_var->svalue == NULL) ? NULL :
pg_strdup(source_var->svalue);
About:
+ if (doRetry(st, &now))
+ st->state = CSTATE_RETRY;
+ else
+ st->state = CSTATE_FAILURE;
-> st->state = doRetry(st, &now) ? CSTATE_RETRY : CSTATE_FAILURE;
These lines are quite long - do you suggest to wrap them this way?
+ dest_var->svalue = ((source_var->svalue == NULL) ? NULL :
+
pg_strdup(source_var->svalue));
+ st->state = (doRetry(st, &now)
? CSTATE_RETRY :
+
CSTATE_FAILURE);
+ if (sqlState) -> if (sqlState != NULL) ?
Ok!
Function getTransactionStatus name does not seem to correspond fully to
what the
function does. There is a passthru case which should be either avoided
or
clearly commented.
I don't quite understand you - do you mean that in fact this function
finds out whether we are in a (failed) transaction block or not? Or do
you mean that the case of PQTRANS_INTRANS is also ok?...
About:
- commandFailed(st, "SQL", "perhaps the backend died while
processing");
+ clientAborted(st,
+ "perhaps the backend died while processing");
keep on one line?
I tried not to break the limit of 80 characters, but if you think that
this is better, I'll change it.
Overall, the comment text in StatsData is very clear. However they are
not
clearly linked to the struct fields. I'd suggest that earch field when
used
should be quoted, so as to separate English from code, and the struct
name
should always be used explicitely when possible.
Ok!
I'd insist in a comment that "cnt" does not include "skipped"
transactions
(anymore).
If you mean CState.cnt I'm not sure if this is practically useful
because the code uses only the sum of all client transactions including
skipped and failed... Maybe we can rename this field to nxacts or
total_cnt?
About v11-4. I'm do not feel that these changes are very
useful/important for now. I'd propose that your prioritize on updating
11-3 so that we can have another round about it as soon as possible,
and keep that one later.
Ok!
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
