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

Hello, hackers!

This is the eleventh version of the patch for error handling and
retrying of transactions with serialization/deadlock failures in pgbench
(based on the commit 14e9b2a752efaa427ce1b400b9aaa5a636898a04) thanks to
the comments of Fabien Coelho and Arthur Zakirov in this thread.

v11-0001-Pgbench-errors-use-the-RandomState-structure-for.patch
- a patch for the RandomState structure (this is used to reset a
client's random seed during the repeating of transactions after
serialization/deadlock failures).

v11-0002-Pgbench-errors-use-the-Variables-structure-for-c.patch
- a patch for the Variables structure (this is used to reset client
variables during the repeating of transactions after
serialization/deadlock failures).

v11-0003-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).

v11-0004-Pgbench-errors-use-a-separate-function-to-report.patch
- a patch for a separate error reporting function (this is used to
report client failures that do not cause an aborts and this depends on
the level of debugging). Although this is a try to fix a duplicate code
for debug messages (see [1]), this may seem mostly refactoring and
therefore may not seem very necessary for this set of patches (see [2],
[3]), so this patch becomes the last as an optional.

Any suggestions are welcome!

[1]
https://www.postgresql.org/message-id/20180405180807.0bc1114f%40wp.localdomain

> There is a lot of checks like "if (debug_level >= DEBUG_FAILS)" with
> corresponding fprintf(stderr..) I think it's time to do it like in the
> main code, wrap with some function like log(level, msg).

[2]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808071823540.13466%40lancre

> However ISTM that it is not as necessary as the previous one, i.e. we
> could do without it to get the desired feature, so I see it more as a
> refactoring done "in passing", and I'm wondering whether it is
> really worth it because it adds some new complexity, so I'm not sure of
> the net benefit.

[3]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808101027390.9120%40lancre

> I'm still not over enthousiastic with these changes, and still think
> that
> it should be an independent patch, not submitted together with the
> "retry
> on error" feature.

All that was fixed from the previous version:

[4]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808071823540.13466%40lancre

> I'm at odds with the proposed levels. ISTM that pgbench internal
> errors which warrant an immediate exit should be dubbed "FATAL",

> I'm unsure about the "log_min_messages" variable name, I'd suggest
> "log_level".
>
> I do not see the asserts on LOG >= log_min_messages as useful, because
> the level can only be LOG or DEBUG anyway.

> * PQExpBuffer
>
> I still do not see a positive value from importing PQExpBuffer
> complexity and cost into pgbench, as the resulting code is not very
> readable and it adds malloc/free cycles, so I'd try to avoid using
> PQExpBuf as much as possible. ISTM that all usages could be avoided in
> the patch, and most should be avoided even if ExpBuffer is imported
> because it is really useful somewhere.
>
> - to call pgbench_error from pgbench_simple_error, you can do a
> pgbench_log_va(level, format, va_list) version called both from
> pgbench_error & pgbench_simple_error.
>
> - for PGBENCH_DEBUG function, do separate calls per type, the very
> small partial code duplication is worth avoiding ExpBuf IMO.
>
> - for doCustom debug: I'd just let the printf as it is, with a
> comment, as it is really very internal stuff for debug. Or I'd just
> snprintf a something in a static buffer.
>
> ...
>
> - for listAvailableScript: I'd simply call "pgbench_error(LOG" several
> time, once per line.
>
> I see building a string with a format (printfExpBuf..) and then
> calling the pgbench_error function with just a "%s" format on the
> result as not very elegant, because the second format is somehow
> hacked around.

[5]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808101027390.9120%40lancre

> I suggest that the called function does only one simple thing,
> probably "DEBUG", and that the *caller* prints a message if it is
> unhappy
> about the failure of the called function, as it is currently done. This
> allows to provide context as well from the caller, eg "setting variable
> %s
> failed while <some specific context>". The user call rerun under debug
> for
> precision if they need it.

[6]
https://www.postgresql.org/message-id/20180810125327.GA2374%40zakirov.localdomain

> I agree with Fabien. Calling pgbench_error() inside pgbench_error()
> could be dangerous. I think "fmt" checking could be removed, or we may
> use Assert() or fprintf()+exit(1) at least.

[7]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808121057540.6189%40lancre

> * typo in comments: "varaibles"
>
> * About enlargeVariables:
>
> multiple INT_MAX error handling looks strange, especially as this code
> can
> never be triggered because pgbench would be dead long before having
> allocated INT_MAX variables. So I would not bother to add such checks.

> I'm not sure that the size_t cast here and there are useful for any
> practical values likely to be encountered by pgbench.
>
> The exponential allocation seems overkill. I'd simply add a constant
> number of slots, with a simple rule:
>
> /* reallocated with a margin */
> if (max_vars < needed) max_vars = needed + 8;

[8]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808151046090.30050%40lancre

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

> I do not think that the RETRIES_ENABLED macro is a good thing. I'd
> suggest
> to write the condition four times.
>
> 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'd suggest to put "ANOTHER_SQL_FAILURE" as the last option, otherwise
> "another"
> does not make sense yet. I'd suggest to name it "OTHER_SQL_FAILURE".

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

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

> commandFailed: I'm not thrilled by the added boolean, which is
> partially
> redundant with the second argument.
>
> 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.

[9]
https://www.postgresql.org/message-id/alpine.DEB.2.21.1808170917510.20841%40lancre

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

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