Re: WIP Patch: Pgbench Serialization and deadlock errors

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Marina Polyakova <m(dot)polyakova(at)postgrespro(dot)ru>
Cc: PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: WIP Patch: Pgbench Serialization and deadlock errors
Date: 2017-07-01 14:39:08
Message-ID: alpine.DEB.2.20.1707011607370.6840@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Hello Marina,

A few comments about the submitted patches.

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

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

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

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.

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

About 0001:

In accumStats, just use one level if, the two levels bring nothing.

In doLog, added columns should be at the end of the format. 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.

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.

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

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.

About 0004:

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

--
Fabien.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-07-01 19:53:02 PostgresNode::poll_query_until hacking
Previous Message Masahiko Sawada 2017-07-01 04:39:27 Re: Small comment fix in partition.c