Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors - Mailing list pgsql-hackers

> Hello,

Hi! I'm very sorry that I did not answer for so long, I was very busy in 
the release of Postgres Pro 10 :(

>> Here is the third version of the patch for pgbench thanks to Fabien 
>> Coelho comments. As in the previous one, transactions with 
>> serialization and deadlock failures are rolled back and retried until 
>> they end successfully or their number of tries reaches maximum.
> 
> Here is some partial review.

Thank you very much for it!

> It compiles with warnings, please fix them:
> 
>   pgbench.c:2624:28: warning: ‘failure_status’ may be used
> uninitialized in this function
>   pgbench.c:2697:34: warning: ‘command’ may be used uninitialized in
> this function

Ok!

> I do not think that the error handling feature needs preeminence in the
> final report, compare to scale, number of clients and so. The number
> of tries should be put further on.

I added it here only because both this field and field "transaction 
type" are transaction characteristics. I have some doubts where to add 
it. On the one hand, the number of clients, the number of transactions 
per client and the number of transactions actually processed form a good 
logical block which I don't want to divide. On the other hand, the 
number of clients and the number of transactions per client are 
parameters, but the number of transactions actually processed is one of 
the program results. Where, in your opinion, would it be better to add 
the maximum number of transaction tries?

> I would spell "number of tries" instead of "tries number" which seems 
> to
> suggest that each try is attributed a number. "sql" -> "SQL".

Ok!

> For the per statement latency final report, I do not think it is worth
> distinguishing the kind of retry at this level, because ISTM that
> serialization & deadlocks are unlikely to appear simultaneously. I
> would just report total failures and total tries on this report. We
> only have 2 errors now, but if more are added I'm pretty sure that we
> would not want to have more columns...

Thanks, I agree with you.

> Moreover the 25 characters
> alignment is ugly, better use a much smaller alignment.

The variables for the numbers of failures and retries are of type int64 
since the variable for the total number of transactions has the same 
type. That's why such a large alignment (as I understand it now, enough 
20 characters). Do you prefer floating alignemnts, depending on the 
maximum number of failures/retries for any command in any script?

> I'm okay with having details shown in the "log to file" group report.

I think that the output format of retries statistics should be same 
everywhere, so I would just like to output the total number of retries 
here.

> The documentation does not seem consistent. It discusses "the very last 
> fields"
> and seem to suggest that there are two, but the example trace below 
> just
> adds one field.

I'm sorry, I do not understand what you are talking about. I used 
commands and the files from the end of your message ("psql < 
dl_init.sql" and "pgbench -f dl_trans.sql -c 8 -T 10 -P 1"), and I got 
this output from pgbench:

starting vacuum...ERROR:  relation "pgbench_branches" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_tellers" does not exist
(ignoring this error and continuing anyway)
ERROR:  relation "pgbench_history" does not exist
(ignoring this error and continuing anyway)
end.
progress: 1.0 s, 14.0 tps, lat 9.094 ms stddev 5.304
progress: 2.0 s, 25.0 tps, lat 284.934 ms stddev 450.692, 1 failed
progress: 3.0 s, 21.0 tps, lat 337.942 ms stddev 473.210, 1 failed
progress: 4.0 s, 11.0 tps, lat 459.041 ms stddev 499.908, 2 failed
progress: 5.0 s, 28.0 tps, lat 220.219 ms stddev 411.390, 2 failed
progress: 6.0 s, 5.0 tps, lat 402.695 ms stddev 492.526, 2 failed
progress: 7.0 s, 24.0 tps, lat 343.249 ms stddev 626.181, 2 failed
progress: 8.0 s, 14.0 tps, lat 505.396 ms stddev 501.836, 1 failed
progress: 9.0 s, 40.0 tps, lat 180.080 ms stddev 381.335, 1 failed
progress: 10.0 s, 1.0 tps, lat 0.000 ms stddev 0.000, 1 failed
transaction type: dl_trans.sql
transaction maximum tries number: 1
scaling factor: 1
query mode: simple
number of clients: 8
number of threads: 1
duration: 10 s
number of transactions actually processed: 191
number of failures: 14 (7.330 %)
latency average = 356.701 ms
latency stddev = 564.942 ms
tps = 18.735807 (including connections establishing)
tps = 18.744898 (excluding connections establishing)

As I understand it, in the documentation "the very last fields" refer to 
the aggregation logging which is not used here. So what's the problem?

> If you want a paragraph you should add <para>, skipping a line does not
> work (around "All values are computed for ...").

Sorry, thanks =[

> I do not understand the second note of the --max-tries documentation.
> It seems to suggest that some script may not end their own 
> transaction...
> which should be an error in my opinion? Some explanations would be 
> welcome.

As you told me here [1], "I disagree about exit in ParseScript if the 
transaction block is not completed <...> and would break an existing 
feature.". Maybe it's be better to say this:

In pgbench you can use scripts in which the transaction blocks do not 
end. Be careful in this case because transactions that span over more 
than one script are not rolled back and will not be retried in case of 
an error. In such cases, the script in which the error occurred is 
reported as failed.

?

> I'm not sure that "Retries" deserves a type of its own for two 
> counters.

Ok!

> The "retries" in RetriesState may be redundant with these.

The "retries" in RetriesState have a different goal: they sum up not all 
the retries during the execution of the current script but the retries 
for the current transaction.

> The failures are counted on simple counters while retries have a type,
> this is not consistent. I suggest to just use simple counters 
> everywhere.

Ok!

> I'm ok with having the detail report tell about failures & retries only
> when some occured.

Ok!

> typo: sucessufully -> successfully

Thanks! =[

> If a native English speaker could provide an opinion on that, and more
> generally review the whole documentation, it would be great.

I agree with you))

> I think that the rand functions should really take a random_state 
> pointer
> argument, not a Thread or Client.

Thanks, I agree.

> I'm at odds that FailureStatus does not have a clean NO_FAILURE state,
> and that it is merged with misc failures.

:) It is funny but for the code it really did not matter)

> I'm not sure that initRetries, mergeRetries, getAllRetries really
> deserve a function.

Ok!

> I do not thing that there should be two accum Functions. Just extend
> the existing one, and adding zero to zero is not a problem.

Ok!

> I guess that in the end pgbench & psql variables will have to be merged
> if pgbench expression engine is to be used by psql as well, but this is
> not linked to this patch.

Ok!

> The tap tests seems over-complicated and heavy with two pgbench run in
> parallel... I'm not sure we really want all that complexity for this
> somehow small feature. Moreover pgbench can run several scripts, I'm 
> not
> sure why two pgbench would need to be invoked. Could something much
> simpler and lighter be proposed instead to test the feature?

Firstly, two pgbench need to be invoked because we don't know which of 
them will get a deadlock failure. Secondly, I tried much simplier tests 
but all of them failed sometimes although everything was ok:
- tests in which pgbench runs 5 clients and 10 transactions per client 
for a serialization/deadlock failure on any client (sometimes there are 
no failures when it is expected that they will be)
- tests in which pgbench runs 30 clients and 400 transactions per client 
for a serialization/deadlock failure on any client (sometimes there are 
no failures when it is expected that they will be)
- tests in which the psql session starts concurrently and you use sleep
commands to wait pgbench for 10 seconds (sometimes it does not work)
Only advisory locks help me not to get such errors in the tests :(

> The added code does not conform to Pg C style. For instance, if brace
> should be aligned to the if. Please conform the project style.

I'm sorry, thanks =[

> The is_transaction_block_end seems simplistic. ISTM that it would not
> work with compound commands. It should be clearly documented somewhere.

Thanks, I'll fix it.

> Also find attached two scripts I used for some testing:
> 
>   psql < dl_init.sql
>   pgbench -f dl_trans.sql -c 8 -T 10 -P 1

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

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