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

On Wed, 04 Apr 2018 16:07:25 +0300
Marina Polyakova <m(dot)polyakova(at)postgrespro(dot)ru> wrote:

> Hello, hackers!
>
> Here there's a seventh version of the patch for error handling and
> retrying of transactions with serialization/deadlock failures in
> pgbench (based on the commit
> a08dc711952081d63577fc182fcf955958f70add). I added the option
> --max-tries-time which is an implemetation of Fabien Coelho's
> proposal in [1]: the transaction with serialization or deadlock
> failure can be retried if the total time of all its tries is less
> than this limit (in ms). This option can be combined with the option
> --max-tries. But if none of them are used, failed transactions are
> not retried at all.
>
> Also:
> * Now when the first failure occurs in the transaction it is always
> reported as a failure since only after the remaining commands of this
> transaction are executed we find out whether we can try again or not.
> Therefore add the messages about retrying or ending the failed
> transaction to the "fails" debugging level so you can distinguish
> failures (which are retried) and errors (which are not retried).
> * Fix a report on the latency average because the total time includes
> time for both errors and successful transactions.
> * Code cleanup (including tests).
>
> [1]
> https://www.postgresql.org/message-id/alpine.DEB.2.20.1803292134380.16472%40lancre
>
> > Maybe the max retry should rather be expressed in time rather than
> > number
> > of attempts, or both approach could be implemented?
>

Hi, I did a little review of your patch. It seems to work as
expected, documentation and tests are there. Still I have few comments.

There is a lot of checks like "if (debug_level >= DEBUG_FAILS)" with
corresponding fprintf(stderr..) I think it's time to do it like in the
main code, wrap with some function like log(level, msg).

In CSTATE_RETRY state used_time is used only in printing but calculated
more than needed.

In my opinion Debuglevel should be renamed to DebugLevel that looks
nicer, also there DEBUGLEVEl (where last letter is in lower case) which
is very confusing.

I have checked overall functionality of this patch, but haven't checked
any special cases yet.

--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company