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

Thank you very much again!

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.

Yes it is. That's why I wrote several notes about it in documentation where there may be a misunderstanding:

+ Transactions with serialization or deadlock failures (or with both
+        of them if used script contains several transactions; see
+        <xref linkend="transactions-and-scripts"
+ endterm="transactions-and-scripts-title"> for more information) are
+        marked separately and their time is not reported as for skipped
+        transactions.

+ <refsect2 id="transactions-and-scripts">
+ <title id="transactions-and-scripts-title">What is the <quote>Transaction</> Actually Performed in <application>pgbench</application>?</title>

+    If a transaction has serialization and/or deadlock failures, its
+ <replaceable>time</> will be reported as <literal>serialization failure</>,
+   <literal>deadlock failure</>, or
+   <literal>serialization and deadlock failures</>, respectively.
+  <note>
+   <para>
+ Transactions can have both serialization and deadlock failures if the
+     used script contained several transactions.  See
+     <xref linkend="transactions-and-scripts"
+     endterm="transactions-and-scripts-title"> for more information.
+    </para>
+  </note>

+  <note>
+   <para>
+ The number of transactions attempts within the interval can be greater than + the number of transactions within this interval multiplied by the maximum
+    attempts number.  See <xref linkend="transactions-and-scripts"
+    endterm="transactions-and-scripts-title"> for more information.
+   </para>
+  </note>

+       <note>
+ <para>The total sum of per-command failures of each type can be greater
+         than the number of transactions with reported failures.
+         See <xref linkend="transactions-and-scripts"
+ endterm="transactions-and-scripts-title"> for more information.
+         </para>
+       </note>

And I didn't make rollbacks to savepoints after the failure because they cannot help for serialization failures at all: after rollback to savepoint a new attempt will be always unsuccessful.

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.

Thank you, I'll change it.

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

Glad to hear it!

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

I'm not sure that it is enough. Firstly it may be several transactions in script so to count the average attempts number you should know the total number of runned transactions. Secondly I think that stddev for attempts number can be quite interesting and often it is not close to zero.

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.

Thanks, I agree that it's better to do in this way.

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

I agree with you. I'll change it.

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.

Do you mean the code cleanup for doCustom function? Because if I do so there will be two code styles for state blocks and their variables in this function..

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 divided these states because if there's a failed transaction block you should end it before retrying. It means to go to states CSTATE_START_COMMAND -> CSTATE_WAIT_RESULT -> CSTATE_END_COMMAND with the appropriate command. How do you propose not to go to these states?

About malloc - I agree with you that it should be done without malloc/free.

About savepoints - as I wrote you earlier I didn't make rollbacks to savepoints after the failure. Because they cannot help for serialization failures at all: after rollback to savepoint a new attempt will be always unsuccessful.

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.

Thanks, I'll fix it for usual transaction blocks that don't end in the scripts.

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

Oh, I'm sorry. I'll fix it too.

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

I think, there may be a misunderstanding. Because script can contain several transactions and get both failures.

* 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

Smaller updates, without dedicated examples, should be enough.

Maybe there should be some examples to prepare people what they can see in the output of the program? Of course now failures are special cases because they disconnect its clients to the end of the program and ruin all the results. I hope that if this patch is committed there will be much more cases with retried failures.

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

Oh, I'm sorry, it is a typo in the documentation.

Marina Polyakova
Postgres Professional:
The Russian Postgres Company

Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to