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

On 09-07-2018 16:05, Fabien COELHO wrote:
> Hello Marina,

Hello, Fabien!

> Here is a review for the last part of your v9 version.

Thank you very much for this!

> Patch does not "git apply" (may anymore):
> error: patch failed: doc/src/sgml/ref/pgbench.sgml:513
> error: doc/src/sgml/ref/pgbench.sgml: patch does not apply

Sorry, I'll send a new version soon.

> However I could get it to apply with the "patch" command.
>
> Then patch compiles, global & pgbench "make check" are ok.

:-)

> Feature
> =======
>
> The patch adds the ability to restart transactions (i.e. the full
> script)
> on some errors, which is a good thing as it allows to exercice postgres
> performance in more realistic scenarii.
>
> * -d/--debug: I'm not in favor in requiring a mandatory text argument
> on this
> option. It is not pratical, the user has to remember it, and it is a
> change.
> I'm sceptical of the overall debug handling changes. Maybe we could
> have
> multiple -d which lead to higher debug level, but I'm not sure that it
> can be
> made to work for this case and still be compatible with the previous
> behavior.
> Maybe you need a specific option for your purpose, eg "--debug-retry"?

As you wrote in [1], adding an additional option is also a bad idea:

> I'm sceptical of the "--debug-fails" options. ISTM that --debug is
> already there
> and should just be reused.

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..

> Code
> ====
>
> * The implementation is less complex that the previous submission,
> which is a good thing. I'm not sure that all the remaining complexity
> is still fully needed.
>
> * 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.

> Leves ELEVEL_LOG_CLIENT_{FAIL,ABORTED} & LOG_MAIN look unclear to me.
> In particular, the "CLIENT" part is not very useful. If the
> distinction makes sense, I would have kept "LOG" for the initial one
> and
> add other ones for ABORT and PGBENCH, maybe.

Ok!

> * There are no comments about "retries" in StatData, CState and Command
> structures.
>
> * Also, for StatData, I would like to understand the logic between cnt,
> skipped, retries, retried, errors, ... so a clear information about the
> expected invariant if any would be welcome. One has to go in the code
> to
> understand how these fields relate one to the other.
>
> <...>
>
> * How "errors" differs from "ecnt" is unclear to me.

Thank you, I'll fix this.

> * commandFailed: I think that it should be kept much simpler. In
> particular, having errors on errors does not help much: on
> ELEVEL_FATAL, it ignores the actual reported error and generates
> another error of the same level, so that the initial issue is hidden.
> Even if these are can't happen cases, hidding the origin if it occurs
> looks unhelpful. Just print it directly, and maybe abort if you think
> that it is a can't happen case.

Oh, thanks, my mistake(

> * copyRandomState: just use sizeof(RandomState) instead of making
> assumptions
> about the contents of the struct. Also, this function looks pretty
> useless,
> why not just do a plain assignment?
>
> * copyVariables: lacks comments to explain that the destination is
> cleaned up
> and so on. The cleanup phase could probaly be in a distinct function,
> so that
> the code would be clearer. Maybe the function variable names are too
> long.

Thank you, I'll fix this.

> if (current_source->svalue)
>
> in the context of a guard for a strdup, maybe:
>
> if (current_source->svalue != NULL)

I'm sorry, I'll fix this.

> * I do not understand the comments on CState enum: "First, remember the
> failure
> in CSTATE_FAILURE. Then process other commands of the failed
> transaction if any"
> Why would other commands be processed at all if the transaction is
> aborted?
> For me any error must leads to the rollback and possible retry of the
> transaction. This comment needs to be clarified. It should also say
> that on FAILURE, it will go either to RETRY or ABORTED. See below my
> comments about doCustom.
>
> It is unclear to me why their could be several failures within a
> transaction, as I would have stopped that it would be aborted on the
> first one.
>
> * I do not undestand the purpose of first_failure. The comment should
> explain
> why it would need to be remembered. From my point of view, I'm not
> fully
> convinced that it should.
>
> <...>
>
> * executeCondition: this hides client automaton state changes which
> were
> clearly visible beforehand in the switch, and the different handling of
> if & elif is also hidden.
>
> I'm against this unnecessary restructuring and to hide such an
> information,
> all state changes should be clearly seen in the state switch so that it
> is
> easier to understand and follow.
>
> I do not see why touching the conditional stack on internal errors
> (evaluateExpr failure) brings anything, the whole transaction will be
> aborted
> anyway.
>
> * doCustom changes.
>
> On CSTATE_START_COMMAND, it considers whether to retry on the end.
> For me, this cannot happen: if some command failed, then it should have
> skipped directly to the RETRY state, so that you cannot get to the end
> of the script with an error. Maybe you could assert that the state of
> the
> previous command is NO_FAILURE, though.
>
> 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?
>
> Function executeCondition is a bad idea, as stated above.

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?

> 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, if you
use the option --latency-limit, you we will never know in advance
whether the serialization/deadlock failure will be retried or not. 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?..

> * reporting
>
> The number of transactions above the latency limit report can be
> simplified.
> Remove the if and just use one printf f with a %s for the optional
> comment.
> I'm not sure this optional comment is useful there.

Oh, thanks, my mistake(

> Before the patch, ISTM that all lines relied on one printf. you have
> changed to a style where a collection of printf is used to compose a
> line. I'd suggest to keep to the previous one-printf-prints-one-line
> style, where possible.

Ok!

> 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]:
> 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?

> Some people try to parse the output, so it should be deterministic. I'd
> add
> the needed columns always if appropriate (i.e. under retry), even if
> none
> occured.

Ok!

> * processXactStats: An else is replaced by a detailed stats, with the
> initial
> "no detailed stats" comment kept. The function is called both in the
> thenb
> & else branch. The structure does not make sense anymore. I'm not sure
> this changed was needed.
>
> * getLatencyUsed: declared "double" so "return 0.0".
>
> * typo: ruin -> run; probably others, I did not check for them in
> detail.

Oh, thanks, my mistakes(

> TAP Tests
> =========
>
> On my laptop, tests last 5.5 seconds before the patch, and about 13
> seconds
> after. This is much too large. Pgbench TAP tests do not deserve to take
> over
> twice as much time as before just on this patch.
>
> One reason which explains this large time is there is a new script
> with a new created instance. I'd suggest to append tests to the
> existing 2 scripts, depending on whether they need a running instance
> or not.

Ok! All new tests that do not need a running instance are already added
to the file 002_pgbench_no_server.pl.

> Secondly, I think that the design of the tests are too heavy. For such
> a feature, ISTM enough to check that it works, i.e. one test for
> deadlocks (trigger one or a few deadlocks), idem for serializable,
> maybe idem for other errors if any.
>
> <...>
>
> 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.

> The challenge is to do that reliably and efficiently, i.e. so that the
> test does
> not rely on chance and is still quite efficient.
>
> The trick you use is to run an interactive psql in parallel to pgbench
> so as to
> play with concurrent locks. That is interesting, but deserves more
> comments
> and explanatation, eg before the test functions.
>
> Maybe this could be achieved within pgbench by using some wait stuff
> in PL/pgSQL so that concurrent client can wait one another based on
> data in unlogged table updated by a CALL within an "embedded"
> transactions? Not sure.
>
> <...>
>
> Anyway, TAP tests should be much lighter (in total time), and if
> possible much simpler.

I'll try, thank you..

> Otherwise, maybe (simple) pgbench-side thread
> barrier could help, but this would require more thinking.

Tests must pass if we use --disable-thread-safety..

> Documentation
> =============
>
> Not looked at in much details for now. Just a few comments:
>
> Having the "most important settings" on line 1-6 and 8 (i.e. skipping
> 7) looks
> silly. The important ones should simply be the first ones, and the 8th
> is not
> that important, or it is in 7th position.

Ok!

> 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 (to
report errors and possibly retries of this type in this statement) and
we already have 2 new columns for all errors and retries. So I'm not
sure that we need add anything other than statistics only about all the
errors and all the retries in general.

> The documentation should state clearly what
> are the counted errors, and then what are their effects on the reported
> stats.
> The "Errors and Serialization/Deadlock Retries" section is a good start
> in that
> direction, but it does not talk about pgbench internal errors (eg
> "cos(true)").
> I think it should more explicit about errors.

Thank you, I'll try to improve it.

> Option --max-tries default value should be spelled out in the doc.

If you mean that it is set to 1 if neither of the options --max-tries or
--latency-limit is explicitly used, I'll fix this.

> "Client's run is aborted", do you mean "Pgbench run is aborted"?

No, other clients continue their run as usual.

> * FailureStatus states are not homogeneously named. I'd suggest to use
> *_FAILURE for all cases. The miscellaneous case should probably be the
> last. I do not understand the distinction between ANOTHER_FAILURE &
> IN_FAILED_SQL_TRANSACTION. Why should it be needed? [again, see and of
> review]
>
> <...>
>
> "If a failed transaction block does not terminate in the current
> script":
> this just looks like a very bad idea, and explains my general ranting
> above about this error condition. ISTM that the only reasonable option
> is that a pgbench script should be inforced as a transaction, or a set
> of
> transactions, but cannot be a "piece" of transaction, i.e. pgbench
> script
> with "BEGIN;" but without a corresponding "COMMIT" is a user error and
> warrants an abort, so that there is no need to manage these "in aborted
> transaction" errors every where and report about them and document them
> extensively.
>
> This means adding a check when a script is finished or starting that
> PQtransactionStatus(const PGconn *conn) == PQTRANS_IDLE, and abort if
> not
> with a fatal error. Then we can forget about these "in tx errors"
> counting,
> reporting and so on, and just have to document the restriction.

Ok!

[1]
https://www.postgresql.org/message-id/alpine.DEB.2.20.1801031720270.20034%40lancre
[2]
https://www.postgresql.org/message-id/e4c5e8cefa4a8e88f1273b0f1ee29e56@postgrespro.ru

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