|From:||Marina Polyakova <m(dot)polyakova(at)postgrespro(dot)ru>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, pgsql-hackers(at)postgresql(dot)org, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, Teodor Sigaev <teodor(at)sigaev(dot)ru>|
|Subject:||Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 11-07-2018 16:24, Fabien COELHO wrote:
> Hello Marina,
>>> * -d/--debug: I'm not in favor in requiring a mandatory text argument
>>> on this option.
>> As you wrote in , adding an additional option is also a bad idea:
> Hey, I'm entitled to some internal contradictions:-)
... and discussions will be continue forever %-)
>>> I'm sceptical of the "--debug-fails" options. ISTM that --debug is
>>> already there and should just be reused.
> I was thinking that you could just use the existing --debug, not
> change its syntax. My point was that --debug exists, and you could
> just print
> the messages when under --debug.
Now I understand you better, thanks. I think it will be useful to
receive only messages about failures, because they and progress reports
can be lost in many other debug messages such as "client %d sending ..."
/ "client %d executing ..." / "client %d receiving".
>> Maybe it's better to use an optional argument/arguments for
>> compatibility (--debug[=fails] or --debug[=NUM])? But if we use the
>> numbers, now I can see only 2 levels, and there's no guarantee that
>> they will no change..
> Optional arguments to options (!) are not really clean things, so I'd
> like to avoid going onto this path, esp. as I cannot see any other
> instance in pgbench or elsewhere in postgres,
AFAICS they are used in pg_waldump (option --stats[=record]) and in psql
> and I personnaly
> consider these as a bad idea.
> So if absolutely necessary, a new option is still better than changing
> --debug syntax. If not necessary, then it is better:-)
>>> * I'm reserved about the whole ereport thing, see comments in other
>> Thank you, I'll try to implement the error reporting in the way you
> Dunno if it is a good idea either. The committer word is the good one
> in the end:-à
I agree with you that ereport has good reasons to be non-trivial in the
backend and it does not have the same in pgbench..
>>> * doCustom changes.
>>> On CSTATE_FAILURE, the next command is possibly started. Although
>>> there is some consistency with the previous point, I think that it
>>> totally breaks the state automaton where now a command can start
>>> while the whole transaction is in failing state anyway. There was no
>>> point in starting it in the first place.
>>> So, for me, the FAILURE state should record/count the failure, then
>>> to RETRY if a retry is decided, else proceed to ABORT. Nothing else.
>>> This is much clearer that way.
>>> Then RETRY should reinstate the global state and proceed to start the
>>> *first* command again.
>>> It is unclear to me why backslash command errors are turned to
>>> instead of ABORTED: there is no way they are going to be retried, so
>>> maybe they should/could skip directly to ABORTED?
>> So do you propose to execute the command "ROLLBACK" without
>> calculating its latency etc. if we are in a failed transaction and
>> clear the conditional stack after each failure?
>> Also just to be clear: do you want to have the state CSTATE_ABORTED
>> for client abortion and another state for interrupting the current
> I do not understand what "interrupting the current transaction" means.
> A transaction is either committed or rollbacked, I do not know about
I mean that IIUC the server usually only reports the error and you must
manually send the command "END" or "ROLLBACK" to rollback a failed
> When it is rollbacked, probably some stats will be
> collected in passing, I'm fine with that.
> If there is an error in a pgbench script, the transaction is aborted,
> which means for me that the script execution is stopped where it was,
> and either it is restarted from the beginning (retry) or counted as
> failure (not retry, just aborted, really).
> If by interrupted you mean that one script begins a transaction and
> another ends it, as I said in the review I think that this strange
> case should be forbidden, so that all the code and documentation
> trying to
> manage that can be removed.
>>> The current RETRY state does memory allocations to generate a message
>>> with buffer allocation and so on. This looks like a costly and
>>> operation. If the user required "retries", then this is normal
>>> the retries are counted and will be printed out in the final report,
>>> and there is no point in printing out every single one of them.
>>> Maybe you want that debugging, but then coslty operations should be
>> I think we need these debugging messages because, for example,
> Debugging message should cost only when under debug. When not under
> debug, there should be no debugging message, and there should be no
> cost for building and discarding such messages in the executed code
> path beyond
> testing whether program is under debug.
>> if you use the option --latency-limit, you we will never know in
>> advance whether the serialization/deadlock failure will be retried or
> ISTM that it will be shown final report. If I want debug, I ask for
> --debug, otherwise I think that the command should do what it was
> asked for, i.e. run scripts, collect performance statistics and show
> them at the end.
> In particular, when running with retries is enabled, the user is
> expecting deadlock/serialization errors, so that they are not "errors"
> as such for
>> They also help to understand which limit of retries was violated or
>> how close we were to these limits during the execution of a specific
>> transaction. But I agree with you that they are costly and can be
>> skipped if the failure type is never retried. Maybe it is better to
>> split them into multiple error function calls?..
> Debugging message costs should only be incurred when under --debug,
> not otherwise.
Ok! IIUC instead of this part of the code
"client %d repeats the failed transaction (try %d",
st->id, st->retries + 1);
appendPQExpBuffer(&errmsg_buf, "/%d", max_tries);
", %.3f%% of the maximum time of tries was used",
pgbench_error(DEBUG_FAIL, "%s", errmsg_buf.data);
can we try something like this?
PGBENCH_ERROR("client %d repeats the failed transaction (try %d",
st->id, st->retries + 1);
PGBENCH_ERROR(", %.3f%% of the maximum time of tries was used",
>>> You have added 20-columns alignment prints. This looks like too much
>>> generates much too large lines. Probably 10 (billion) would be
>> I have already asked you about this in :
>>> The variables for the numbers of failures and retries are of type
>>> since the variable for the total number of transactions has the same
>>> type. That's why such a large alignment (as I understand it now,
>>> 20 characters). Do you prefer floating alignemnts, depending on the
>>> maximum number of failures/retries for any command in any script?
> An int64 counter is not likely to reach its limit anytime soon:-) If
> the column display limit is ever reached, ISTM that then the text is
> just misaligned, which is a minor and rare inconvenience. If very wide
> columns are used, then it does not fit my terminal and the report text
> will always be wrapped around, which makes it harder to read, every
>>> The latency limit to 900 ms try is a bad idea because it takes a lot
>>> of time. I did such tests before and they were removed by Tom Lane
>>> because of determinism and time issues. I would comment this test out
>>> for now.
>> Ok! If it doesn't bother you - can you tell more about the causes of
>> these determinism issues?.. Tests for some other failures that cannot
>> be retried are already added to 001_pgbench_with_server.pl.
> Some farm animals are very slow, so you cannot really assume much
> about time one way or another.
>>> I do not understand why there is so much text about in failed sql
>>> transaction stuff, while we are mainly interested in serialization &
>>> deadlock errors, and this only falls in some "other" category. There
>>> seems to be more details about other errors that about deadlocks &
>>> serializable errors.
>>> The reporting should focus on what is of interest, either all errors,
>>> or some detailed split of these errors.
>>> * "errors_in_failed_tx" is some subcounter of "errors", for a special
>>> case. Why it is there fails me [I finally understood, and I think it
>>> should be removed, see end of review]. If we wanted to distinguish,
>>> then we should distinguish homogeneously: maybe just count the
>>> different error types, eg have things like "deadlock_errors",
>>> "serializable_errors", "other_errors", "internal_pgbench_errors"
>>> would be orthogonal one to the other, and "errors" could be
>>> from these.
>> Thank you, I agree with you. Unfortunately each new error type adds a
>> new 1 or 2 columns of maximum width 20 to the per-statement report
> The fact that some data are collected does not mean that they should
> all be reported in detail. We can have detailed error count and report
> the sum of this errors for instance, or have some more
> verbose/detailed reports
> as options (eg --latencies does just that).
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
|Next Message||Andres Freund||2018-07-11 17:02:44||Re: In pageinspect, perform clean-up after testing gin-related functions|
|Previous Message||Andrey Borodin||2018-07-11 16:04:44||Re: GiST VACUUM|