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

From: Marina Polyakova <m(dot)polyakova(at)postgrespro(dot)ru>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, pgsql-hackers(at)postgresql(dot)org, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, Teodor Sigaev <teodor(at)sigaev(dot)ru>
Subject: Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Date: 2018-07-12 08:57:01
Message-ID: cb2cde10e4e7a10a38b48e9cae8fbd28@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 11-07-2018 21:04, Alvaro Herrera wrote:
> Just a quick skim while refreshing what were those error reporting API
> changes about ...

Thank you!

> On 2018-May-21, Marina Polyakova wrote:
>
>> v9-0001-Pgbench-errors-use-the-RandomState-structure-for-.patch
>> - a patch for the RandomState structure (this is used to reset a
>> client's
>> random seed during the repeating of transactions after
>> serialization/deadlock failures).
>
> LGTM, though I'd rename the random_state struct members so that it
> wouldn't look as confusing. Maybe that's just me.

IIUC, do you like "xseed" instead of "data"?

typedef struct RandomState
{
- unsigned short data[3];
+ unsigned short xseed[3];
} RandomState;

Or do you want to rename "random_state" in the structures RetryState /
CState / TState? Thanks to Fabien Coelho' comments in [1], TState can
contain several RandomStates for different purposes, something like
this:

/*
* Thread state
*/
typedef struct
{
...
/*
* Separate randomness for each thread. Each thread option uses its own
* random state to make all of them independent of each other and
therefore
* deterministic at the thread level.
*/
RandomState choose_script_rs; /* random state for selecting a script */
RandomState throttling_rs; /* random state for transaction throttling
*/
RandomState sampling_rs; /* random state for log sampling */
...
} TState;

>> v9-0002-Pgbench-errors-use-the-Variables-structure-for-cl.patch
>> - a patch for the Variables structure (this is used to reset client
>> variables during the repeating of transactions after
>> serialization/deadlock
>> failures).
>
> Please don't allocate Variable structs one by one. First time allocate
> some decent number (say 8) and then enlarge by duplicating size. That
> way you save realloc overhead. We use this technique everywhere else,
> no reason do different here. Other than that, LGTM.

Ok!

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

> While reading your patch, it occurs to me that a run is not
> deterministic
> at the thread level under throttling and sampling, because the random
> state is sollicited differently depending on when transaction ends.
> This
> suggest that maybe each thread random_state use should have its own
> random
> state.

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2018-07-12 08:59:02 Re: BUG #15212: Default values in partition tables don't work as expected and allow NOT NULL violation
Previous Message Amit Langote 2018-07-12 08:44:48 Re: Problem on pg_dump RANGE partition with expressions