MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Thu, 12 Jul 2018 11:57:01 +0300
From: Marina Polyakova <m.polyakova@postgrespro.ru>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>, pgsql-hackers@postgresql.org, Ildus
 Kurbangaliev <i.kurbangaliev@postgrespro.ru>, Teodor Sigaev
 <teodor@sigaev.ru>
Subject: Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Organization: Postgres Professional
In-Reply-To: <20180711180417.3ytmmwmonsr5lra7@alvherre.pgsql>
References: <20180711180417.3ytmmwmonsr5lra7@alvherre.pgsql>
Message-ID: <cb2cde10e4e7a10a38b48e9cae8fbd28@postgrespro.ru>
User-Agent: Roundcube Webmail/1.2.4
Precedence: bulk

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