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

On 17-08-2018 10:49, Fabien COELHO wrote:
> Hello Marina,
>
>>> 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:
>> [...]
>> 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;
>
> Ok, not always the same point, and you confirm that it identifies
> where the error is raised which leads to a retry.

Yes, I confirm this. I'll try to write more clearly about this in the
documentation...

>> 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
>
> Here is an attempt at having a more precise and shorter version, not
> sure it is much better than yours, though:
>
> """
> Transactions are counted depending on their execution and outcome.
> First
> a transaction may have started or not: skipped transactions occur
> under --rate and --latency-limit when the client is too late to
> execute them. Secondly, a started transaction may ultimately succeed
> or fail on some error, possibly after some retries when --max-tries is
> not one. Thus
> """

Thank you!

>>> 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 understand it more as a catch all default "none of the above" case.

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?
>
> Yes. And also I'm not sure we should want this boolean at all.

Perhaps we can use a separate function to print the messages about
client's abortion, something like this (it is assumed that all abortions
happen when processing SQL commands):

static void
clientAborted(CState *st, const char *message)
{
pgbench_error(...,
"client %d aborted in command %d (SQL) of script %d; %s\n",
st->id, st->command, st->use_file, message);
}

Or perhaps we can use a more detailed failure status so for each type of
failure we always know the command name (argument "cmd") and whether the
client is aborted. Something like this (but in comparison with the first
variant ISTM overly complicated):

/*
* For the failures during script execution.
*/
typedef enum FailureStatus
{
NO_FAILURE = 0,

/*
* Failures in meta commands. In these cases the failed transaction is
* terminated.
*/
META_SET_FAILURE,
META_SETSHELL_FAILURE,
META_SHELL_FAILURE,
META_SLEEP_FAILURE,
META_IF_FAILURE,
META_ELIF_FAILURE,

/*
* Failures in SQL commands. In cases of serialization/deadlock
failures a
* failed transaction is re-executed from the very beginning if
possible;
* otherwise the failed transaction is terminated.
*/
SERIALIZATION_FAILURE,
DEADLOCK_FAILURE,
OTHER_SQL_FAILURE, /* other failures in SQL commands that are not
* listed by themselves above */

/*
* Failures while processing SQL commands. In this case the client is
* aborted.
*/
SQL_CONNECTION_FAILURE
} FailureStatus;

>> [...]
>> If in such cases one command is placed on several lines, ISTM that the
>> code is more understandable if curly brackets are used...
>
> Hmmm. Such basic style changes are avoided because they break
> backpatching, so we try to avoid gratuitous changes unless there is a
> strong added value, which does not seem to be the case here.

Ok!

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