Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

On 15-08-2018 11:50, Fabien COELHO wrote:
> Hello Marina,

Hello!

>> v10-0004-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).
>
> Patch applies cleanly.
>
> It allows retrying a script (considered as a transaction) on
> serializable and deadlock errors, which is a very interesting
> extension but also impacts pgbench significantly.
>
> I'm waiting for the feature to be right before checking in full the
> documentation and tests. There are still some issues to resolve before
> checking that.
>
> Anyway, tests look reasonable. Taking advantage of of transactions
> control from PL/pgsql is a good use of this new feature.

:-)

> A few comments about the doc.
>
> According to the documentation, the feature is triggered by --max-tries
> and
> --latency-limit. I disagree with the later, because it means that
> having
> latency limit without retrying is not supported anymore.
>
> Maybe you can allow an "unlimited" max-tries, say with special value
> zero,
> and the latency limit does its job if set, over all tries.
>
> Doc: "error in meta commands" -> "meta command errors", for homogeneity
> with
> other cases?
> ...
> Doc: "never occur.." -> "never occur", or eventually "...".
>
> Doc: "Directly client errors" -> "Direct client errors".
> ...
> inTransactionBlock: I disagree with any function other than doCustom
> changing
> the client state, because it makes understanding the state machine
> harder. There
> is already one exception to that (threadRun) that I wish to remove. All
> state
> changes must be performed explicitely in doCustom.
> ...
> PQexec("ROOLBACK"): you are inserting a synchronous command, for which
> the
> thread will have to wait for the result, in a middle of a framework
> which
> takes great care to use only asynchronous stuff so that one thread can
> manage several clients efficiently. You cannot call PQexec there.
> From where I sit, I'd suggest to sendQuery("ROLLBACK"), then switch to
> a new state CSTATE_WAIT_ABORT_RESULT which would be similar to
> CSTATE_WAIT_RESULT, but on success would skip to RETRY or ABORT instead
> of proceeding to the next command.
> ...
> memcpy(&(st->retry_state.random_state), &(st->random_state),
> sizeof(RandomState));
>
> Is there a problem with "st->retry_state.random_state =
> st->random_state;"
> instead of memcpy? ISTM that simple assignments work in C. Idem in the
> reverse
> copy under RETRY.

Thank you, I'll fix this.

> Detailed -r report. I understand from the doc that the retry number on
> the
> detailed per-statement report is to identify at what point errors
> occur?
> Probably this is more or less always at the same point on a given
> script,
> so that the most interesting feature is to report the number of retries
> at the
> script level.

This may depend on various factors.. for example:

transaction type: pgbench_test_serialization.sql
scaling factor: 1
query mode: simple
number of clients: 2
number of threads: 1
duration: 10 s
number of transactions actually processed: 266
number of errors: 10 (3.623%)
number of serialization errors: 10 (3.623%)
number of retried: 75 (27.174%)
number of retries: 75
maximum number of tries: 2
latency average = 72.734 ms (including errors)
tps = 26.501162 (including connections establishing)
tps = 26.515082 (excluding connections establishing)
statement latencies in milliseconds, errors and retries:
0.012 0 0 \set delta random(-5000, 5000)
0.001 0 0 \set x1 random(1, 100000)
0.001 0 0 \set x3 random(1, 2)
0.001 0 0 \set x2 random(1, 1)
19.837 0 0 UPDATE xy1 SET y = y + :delta
WHERE x = :x1;
21.239 5 36 UPDATE xy3 SET y = y + :delta
WHERE x = :x3;
21.360 5 39 UPDATE xy2 SET y = y + :delta
WHERE x = :x2;

And you can always get the number of retries at the script level from
the main report (if only one script is used) or from the report for each
script (if multiple scripts are used).

> I'm still in favor of asserting that the sql connection is idle (no tx
> in
> progress) at the beginning and/or end of a script, and report a user
> error
> if not, instead of writing complex caveats.
>
> If someone has a use-case for that, then maybe it can be changed, but I
> cannot see any in a benchmarking context, and I can see how easy it is
> to have a buggy script with this allowed.
>
> I do not think that the RETRIES_ENABLED macro is a good thing. I'd
> suggest
> to write the condition four times.

Ok!

> ISTM that "skipped" transactions are NOT "successful" so there are a
> problem
> with comments. I believe that your formula are probably right, it has
> more to do
> with what is "success". For cnt decomposition, ISTM that "other
> transactions"
> are really "directly successful transactions".

I agree with you, but I also think that skipped transactions should not
be considered errors. So we can write something like this:

All the transactions are divided into several types depending on their
execution. Firstly, they can be divided into transactions that we
started to execute, and transactions which were skipped (it was too late
to execute them). Secondly, running transactions fall into 2 main types:
is there any command that got a failure during the last execution of the
transaction script or not? Thus

the number of all transactions =
skipped (it was too late to execute them)
cnt (the number of successful transactions) +
ecnt (the number of failed transactions).

A successful transaction can have several unsuccessful tries before a
successfull run. Thus

cnt (the number of successful transactions) =
retried (they got a serialization or a deadlock failure(s), but were
successfully retried from the very beginning) +
directly successfull transactions (they were successfully completed on
the first try).

> I'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, otherwise
> "another"
> does not make sense yet.

Maybe firstly put a general group, and then special cases?...

> I'd suggest to name it "OTHER_SQL_FAILURE".

Ok!

> In TState, field "uint32 retries": maybe it would be simpler to count
> "tries",
> which can be compared directly to max tries set in the option?

If you mean retries in CState - on the one hand, yes, on the other hand,
statistics always use the number of retries...

> ErrorLevel: I have already commented about in review about 10.2. I'm
> not sure of
> the LOG -> DEBUG_FAIL changes. I do not understand the name
> "DEBUG_FAIL", has it
> is not related to debug, they just seem to be internal errors.
> META_ERROR maybe?

As I wrote to you in [1]:

>> I'm at odds with the proposed levels. ISTM that pgbench internal
>> errors which warrant an immediate exit should be dubbed "FATAL",
>
> Ok!
>
>> which
>> would leave the "ERROR" name for... errors, eg SQL errors.
>> ...
>
> The messages of the errors in SQL and meta commands are printed only if
> the option --debug-fails is used so I'm not sure that they should have
> a
> higher error level than main program messages (ERROR vs LOG).

Perhaps we can rename the levels DEBUG_FAIL and LOG to LOG and
LOG_PGBENCH respectively. In this case the client error messages do not
use debug error levels and the term "logging" is already used for
transaction/aggregation logging... Therefore perhaps we can also combine
the options --errors-detailed and --debug-fails into the option
--fails-detailed=none|groups|all_messages. Here --fails-detailed=groups
can be used to group errors in reports or logs by basic types.
--fails-detailed=all_messages can add to this all error messages in the
SQL/meta commands, and messages for processing the failed transaction
(its end/retry).

> The automaton skips to FAILURE on every possible error. I'm wondering
> whether
> it could do so only on SQL errors, because other fails will lead to
> ABORTED
> anyway? If there is no good reason to skip to FAILURE from some errors,
> I'd
> suggest to keep the previous behavior. Maybe the good reason is to do
> some
> counting, but this means that on eg metacommand errors now the script
> would
> loop over instead of aborting, which does not look like a desirable
> change
> of behavior.

Even in the case of meta command errors we must prepare for
CSTATE_END_TX and the execution of the next script: if necessary, clear
the conditional stack and rollback the current transaction block.

> ISTM that it would be more logical to only get into RETRY if there is a
> retry,
> i.e. move the test RETRY/ABORT in FAILURE. For that, instead of
> "canRetry",
> maybe you want "doRetry", which tells that a retry is possible (the
> error
> is serializable or deadlock) and that the current parameters allow it
> (timeout, max retries).
>
> * Minor C style comments:
>
> if / else if / else if ... on *_FAILURE: I'd suggest a switch.
>
> The following line removal does not seem useful, I'd have kept it:
>
> stats->cnt++;
> -
> if (skipped)
>
> copyVariables: I'm not convinced that source_vars & nvars variables are
> that
> useful.

> if (!copyVariables(&st->retry_state.variables, &st->variables)) {
> pgbench_error(LOG, "client %d aborted when preparing to execute a
> transaction\n", st->id);
>
> The message could be more precise, eg "client %d failed while copying
> variables", unless copyVariables already printed a message. As this is
> really
> an internal error from pgbench, I'd rather do a FATAL (direct exit)
> there.
> ISTM that the only possible failure is OOM here, and pgbench is in a
> very bad
> shape if it gets into that.

Ok!

> commandFailed: I'm not thrilled by the added boolean, which is
> partially
> redundant with the second argument.

Do you mean that it is partially redundant with the argument "cmd" and,
for example, the meta commands errors always do not cause the abortions
of the client?

> if (per_script_stats)
> - accumStats(&sql_script[st->use_file].stats, skipped,
> latency, lag);
> + {
> + accumStats(&sql_script[st->use_file].stats, skipped,
> latency, lag,
> + st->failure_status, st->retries);
> + }
> }
>
> I do not see the point of changing the style here.

If in such cases one command is placed on several lines, ISTM that the
code is more understandable if curly brackets are used...

[1]
https://www.postgresql.org/message-id/fcc2512cdc9e6bc49d3b489181f454da%40postgrespro.ru

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company