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 [1], 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
(option --help[=topic]).
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:-)
Ok!
* I'm reserved about the whole ereport thing, see comments in other
messages.
Thank you, I'll try to implement the error reporting in the way you
suggested.
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
skip
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
FAILURE
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
transaction?
I do not understand what "interrupting the current transaction" means.
A transaction is either committed or rollbacked, I do not know about
"interrupted".
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
transaction.
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.
Ok!
The current RETRY state does memory allocations to generate a message
with buffer allocation and so on. This looks like a costly and
useless
operation. If the user required "retries", then this is normal
behavior,
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
guarded.
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
not.
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
them.
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
initPQExpBuffer(&errmsg_buf);
printfPQExpBuffer(&errmsg_buf,
"client %d repeats the failed transaction (try
%d",
st->id, st->retries + 1);
if (max_tries)
appendPQExpBuffer(&errmsg_buf, "/%d", max_tries);
if (latency_limit)
{
appendPQExpBuffer(&errmsg_buf,
", %.3f%% of the maximum time of tries was
used",
getLatencyUsed(st, &now));
}
appendPQExpBufferStr(&errmsg_buf, ")\n");
pgbench_error(DEBUG_FAIL, "%s", errmsg_buf.data);
termPQExpBuffer(&errmsg_buf);
can we try something like this?
PGBENCH_ERROR_START(DEBUG_FAIL)
{
PGBENCH_ERROR("client %d repeats the failed transaction (try %d",
st->id, st->retries + 1);
if (max_tries)
PGBENCH_ERROR("/%d", max_tries);
if (latency_limit)
{
PGBENCH_ERROR(", %.3f%% of the maximum time of tries was used",
getLatencyUsed(st, &now));
}
PGBENCH_ERROR(")\n");
}
PGBENCH_ERROR_END();
You have added 20-columns alignment prints. This looks like too much
and
generates much too large lines. Probably 10 (billion) would be
enough.
I have already asked you about this in [2]:
Probably:-)
The variables for the numbers of failures and retries are of type
int64
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,
enough
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
time.
Ok!
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.
Thanks!
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"
which
would be orthogonal one to the other, and "errors" could be
recomputed
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).
Ok!
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
