Hello Marina,

There's the second version of my patch for pgbench. Now transactions with serialization and deadlock failures are rolled back and retried until they end successfully or their number of attempts reaches maximum.

In details:
- You can set the maximum number of attempts by the appropriate benchmarking option (--max-attempts-number). Its default value is 1 partly because retrying of shell commands can produce new errors.

- Statistics of attempts and failures is printed in progress, in transaction / aggregation logs and in the end with other results (all and for each script). The transaction failure is reported here only if the last retry of this transaction fails.

- Also failures and average numbers of transactions attempts are printed per-command with average latencies if you use the appropriate benchmarking option (--report-per-command, -r) (it replaces the option --report-latencies as I was advised here [1]). Average numbers of transactions attempts are printed only for commands which start transactions.

As usual: TAP tests for new functionality and changed documentation with new examples.

Here are a round of comments on the current version of the patch:

* About the feature

There is a latent issue about what is a transaction. For pgbench a transaction 
is a full script execution.
For postgresql, it is a statement or a BEGIN/END block, several of which may 
appear in a script. From a retry
perspective, you may retry from a SAVEPOINT within a BEGIN/END block... I'm not 
sure how to make general sense
of all this, so this is just a comment without attached action for now.

As the default is not to retry, which is the upward compatible behavior, I 
think that the changes should not
change much the current output bar counting the number of failures.

I would consider using "try/tries" instead of "attempt/attempts" as it is 
shorter. An English native speaker
opinion would be welcome on that point.

* About the code

ISTM that the code interacts significantly with various patches under review or 
ready for committers.
Not sure how to deal with that, there will be some rebasing work...

I'm fine with renaming "is_latencies" to "report_per_command", which is more 
logical & generic.

"max_attempt_number": I'm against typing fields again in their name, aka "hungarian 
naming". I'd suggest
"max_tries" or "max_attempts".

"SimpleStats attempts": I disagree with using this floating poiunt oriented 
structures to count integers.
I would suggest "int64 tries" instead, which should be enough for the purpose.

LastBeginState -> RetryState? I'm not sure why this state is a pointer in CState. Putting the struct would avoid malloc/free cycles. Index "-1" may be used to tell it is not set if necessary.

"CSTATE_RETRY_FAILED_TRANSACTION" -> "CSTATE_RETRY" is simpler and clear enough.

In CState and some code, a failure is a failure, maybe one boolean would be enough. It need only be differentiated when counting, and you have (deadlock_failure || serialization_failure) everywhere.

Some variables, such as "int attempt_number", should be in the client structure, not in the client? Generally, try to use block variables if possible to keep the state clearly disjoints. If there could be NO new variable at the doCustom level that would be great, because that would ensure that there is no machine state mixup hidden in these variables.

I wondering whether the RETRY & FAILURE states could/should be merged:

  on RETRY:
    -> count retry
    -> actually retry if < max_tries (reset client state, jump to command)
    -> else count failure and skip to end of script

The start and end of transaction detection seem expensive (malloc, ...) and assume a one statement per command (what about "BEGIN \; ... \; COMMIT;", which is not necessarily the case, this limitation should be documented. ISTM that the space normalization should be avoided, and something simpler/lighter should be devised? Possibly it should consider handling SAVEPOINT.

I disagree about exit in ParseScript if the transaction block is not completed, especially as it misses out on combined statements/queries (BEGIN \; stuff... \; COMMIT") and would break an existing feature.

There are strange characters things in comments, eg "??ontinuous".

Option "max-attempt-number" -> "max-tries"

I would put the client random state initialization with the state intialization, not with the connection.

* About tracing

Progress is expected to be short, not detailed. Only add the number of failures and retries if max retry is not 1.

* About reporting

I think that too much is reported. I advised to do that, but nevertheless it is a little bit steep.

At least, it should not report the number of tries/attempts when the max number is one. Simple counting should be reported for failures, not floats...

I would suggest a more compact one-line report about failures:

  "number of failures: 12 (0.001%, deadlock: 7, serialization: 5)"

* About the TAP tests

They are too expensive, with 3 initdb. I think that they should be integrated in the existing tests, as a patch has been submitted to rework the whole pgbench tap test infrastructure.

For now, at most one initdb and several small tests inside.

* About the documentation

I'm not sure that the feature needs pre-emminence in the documentation, because most of the time there is no retry as none is needed, there is no failure, so this rather a special (although useful) case for people playing with serializable and other advanced features.

Smaller updates, without dedicated examples, should be enough.

If a transaction is skipped, there was no tries, so the corresponding number of attempts is 0, not one.

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to