From: Marina Polyakova <m.polyakova@postgrespro.ru>
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org, Ildus Kurbangaliev <i.kurbangaliev@postgrespro.ru>
Cc: Teodor Sigaev <teodor@sigaev.ru>
Subject: Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Date: Sun, 10 Jun 2018 00:27:02 +0300
Message-ID: <66cba8455d0c1d5b1bbd4f15f379bbd8@postgrespro.ru> (raw)
In-Reply-To: <alpine.DEB.2.21.1806090810090.5307@lancre>
References: <20180508193621.dphlgwrd5yz67o2z@alvherre.pgsql>
<0569c5309cf536a839e2a3a1433bd113@postgrespro.ru>
<alpine.DEB.2.21.1806090810090.5307@lancre>
On 09-06-2018 9:55, Fabien COELHO wrote:
> Hello Marina,
Hello!
>> 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).
>
> A few comments about this first patch.
Thank you very much!
> Patch applies cleanly, compiles, global & pgbench "make check" ok.
>
> I'm mostly ok with the changes, which cleanly separate the different
> use of random between threads (script choice, throttle delay,
> sampling...) and client (random*() calls).
Glad to hear it :)
> This change is necessary so that a client can restart a transaction
> deterministically (at the client level at least), which is the
> ultimate aim of the patch series.
>
> A few remarks:
>
> The RandomState struct is 6 bytes, which will induce some padding when
> used. This is life and pre-existing. No problem.
>
> ISTM that the struct itself does not need a name, ie. "typedef struct
> { ... } RandomState" is enough.
Ok!
> There could be clear comments, say in the TState and CState structs,
> about what randomness is impacted (i.e. script choices, etc.).
Thank you, I'll add them.
> getZipfianRand, computeHarmonicZipfian: The "thread" parameter was
> justified because it was used for two fieds. As the random state is
> separated, I'd suggest that the other argument should be a zipfcache
> pointer.
I agree with you and I will change it.
> 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.
Thank you, I'll fix this.
> In passing, and totally unrelated to this patch:
>
> I've always been a little puzzled about why a quite small 48-bit
> internal state random generator is used. I understand the need for pg
> to have a portable & state-controlled thread-safe random generator,
> but why this particular small one fails me. The source code
> (src/port/erand48.c, copyright in 1993...) looks optimized for 16 bits
> architectures, which is probably pretty inefficent to run on 64 bits
> architectures. Maybe this could be updated with something more
> consistent with today's processors, providing more quality at a lower
> cost.
This sounds interesting, thanks!
*went to look for a multiplier and a summand that are large enough and
are mutually prime..*
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: m.polyakova@postgrespro.ru, coelho@cri.ensmp.fr, alvherre@2ndquadrant.com, i.kurbangaliev@postgrespro.ru, teodor@sigaev.ru
Subject: Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
In-Reply-To: <66cba8455d0c1d5b1bbd4f15f379bbd8@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox