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

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