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

> Hello Marina,

Hello, Fabien!

> A few comments about the submitted patches.

Thank you very much for them!

> I agree that improving the error handling ability of pgbench is a good
> thing, although I'm not sure about the implications...

Could you tell a little bit more exactly.. What implications are you 
worried about?

> About the "retry" discussion: I agree that retry is the relevant
> option from an application point of view.

I'm glad to hear it!

> ISTM that the retry implementation should be implemented somehow in
> the automaton, restarting the same script for the beginning.

If there are several transactions in this script - don't you think that 
we should restart only the failed transaction?..

> As pointed out in the discussion, the same values/commands should be
> executed, which suggests that random generated values should be the
> same on the retry runs, so that for a simple script the same
> operations are attempted. This means that the random generator state
> must be kept & reinstated for a client on retries. Currently the
> random state is in the thread, which is not convenient for this
> purpose, so it should be moved in the client so that it can be saved
> at transaction start and reinstated on retries.

I think about it in the same way =)

> The number of retries and maybe failures should be counted, maybe with
> some adjustable maximum, as suggested.

If we fix the maximum number of attempts the maximum number of failures 
for one script execution will be bounded above 
(number_of_transactions_in_script * maximum_number_of_attempts). Do you 
think we should make the option in program to limit this number much 
more?

> About 0001:
> 
> In accumStats, just use one level if, the two levels bring nothing.

Thanks, I agree =[

> In doLog, added columns should be at the end of the format.

I have inserted it earlier because these columns are not optional. Do 
you think they should be optional?

> The number
> of column MUST NOT change when different issues arise, so that it
> works well with cut/... unix commands, so inserting a sentence such as
> "serialization and deadlock failures" is a bad idea.

Thanks, I agree again.

> threadRun: the point of the progress format is to fit on one not too
> wide line on a terminal and to allow some simple automatic processing.
> Adding a verbose sentence in the middle of it is not the way to go.

I was thinking about it.. Thanks, I'll try to make it shorter.

> About tests: I do not understand why test 003 includes 2 transactions.
> It would seem more logical to have two scripts.

Ok!

> About 0003:
> 
> I'm not sure that there should be an new option to report failures,
> the information when relevant should be integrated in a clean format
> into the existing reports... Maybe the "per command latency"
> report/option should be renamed if it becomes more general.

I have tried do not change other parts of program as much as possible. 
But if you think that it will be more useful to change the option I'll 
do it.

> About 0004:
> 
> The documentation must not be in a separate patch, but in the same
> patch as their corresponding code.

Ok!

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