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.
</para>
+ <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".
Ok!
"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.
Ok!
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.
Ok!
* 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.
Ok!
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.
Ok!
* 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.
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: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
