Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1glzAf-0003HV-4h for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Jan 2019 16:45:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1glzAd-0006La-Et for pgsql-hackers@arkaria.postgresql.org; Tue, 22 Jan 2019 16:45:03 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1glzAd-0006LT-4P for pgsql-hackers@lists.postgresql.org; Tue, 22 Jan 2019 16:45:03 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1glzAa-00028O-IT for pgsql-hackers@lists.postgresql.org; Tue, 22 Jan 2019 16:45:01 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x0MGivgA009719; Tue, 22 Jan 2019 11:44:57 -0500 From: Tom Lane To: Fabien COELHO cc: PostgreSQL Developers Subject: Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD In-reply-to: References: <9427.1547701450@sss.pgh.pa.us> <21538.1547770868@sss.pgh.pa.us> <4615.1547792324@sss.pgh.pa.us> <29225.1547834182@sss.pgh.pa.us> <7879.1548015981@sss.pgh.pa.us> Comments: In-reply-to Fabien COELHO message dated "Sun, 20 Jan 2019 22:54:41 +0100" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <9661.1548175411.0@sss.pgh.pa.us> Date: Tue, 22 Jan 2019 11:44:57 -0500 Message-ID: <9718.1548175497@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <9661.1548175411.1@sss.pgh.pa.us> Fabien COELHO writes: >>> Here is a POC which defines an internal interface for a PRNG, and use it >>> within pgbench, with several possible implementations which default to >>> rand48. >> I seriously dislike this patch. pgbench's random support is quite >> overengineered already IMO, and this proposes to add a whole batch of >> new code and new APIs to fix a very small bug. > My intention is rather to discuss postgres' PRNG, in passing. Full success > on this point:-) Our immediate problem is to fix a portability failure, which we need to back-patch into at least one released branch, ergo conservatism is warranted. I had in mind something more like the attached. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="dont-use-srandom-in-pgbench-1.patch"; charset="us-ascii" Content-ID: <9661.1548175411.2@sss.pgh.pa.us> Content-Description: dont-use-srandom-in-pgbench-1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index b5c75ce..d5b543f 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -185,7 +185,7 @@ int64 latency_limit =3D 0; char *tablespace =3D NULL; char *index_tablespace =3D NULL; = -/* random seed used when calling srandom() */ +/* random seed used to initialize base_random_sequence */ int64 random_seed =3D -1; = /* @@ -287,6 +287,9 @@ typedef struct RandomState unsigned short xseed[3]; } RandomState; = +/* Various random sequences are initialized from this one. */ +static RandomState base_random_sequence; + /* * Connection state machine states. */ @@ -598,6 +601,8 @@ static const BuiltinScript builtin_script[] =3D = = /* Function prototypes */ +static void initRandomState(RandomState *random_state); +static int64 getrand(RandomState *random_state, int64 min, int64 max); static void setNullValue(PgBenchValue *pv); static void setBoolValue(PgBenchValue *pv, bool bval); static void setIntValue(PgBenchValue *pv, int64 ival); @@ -833,16 +838,28 @@ strtodouble(const char *str, bool errorOK, double *d= v) = /* * Initialize a random state struct. + * + * We derive the seed from base_random_sequence, which must be set up alr= eady. */ static void initRandomState(RandomState *random_state) { - random_state->xseed[0] =3D random(); - random_state->xseed[1] =3D random(); - random_state->xseed[2] =3D random(); + random_state->xseed[0] =3D (unsigned short) + getrand(&base_random_sequence, 0, 0xFFFF); + random_state->xseed[1] =3D (unsigned short) + getrand(&base_random_sequence, 0, 0xFFFF); + random_state->xseed[2] =3D (unsigned short) + getrand(&base_random_sequence, 0, 0xFFFF); } = -/* random number generator: uniform distribution from min to max inclusiv= e */ +/* + * Random number generator: uniform distribution from min to max inclusiv= e. + * + * Although the limits are expressed as int64, you can't generate the ful= l + * int64 range in one call, because the difference of the limits mustn't + * overflow int64. In practice it's unwise to ask for more than an int32 + * range, because of the limited precision of pg_erand48(). + */ static int64 getrand(RandomState *random_state, int64 min, int64 max) { @@ -5126,12 +5143,14 @@ printResults(TState *threads, StatsData *total, in= str_time total_time, } } = -/* call srandom based on some seed. NULL triggers the default behavior. *= / +/* + * Set up a random seed according to seed parameter (NULL means default), + * and initialize base_random_sequence for use in initializing other sequ= ences. + */ static bool set_random_seed(const char *seed) { - /* srandom expects an unsigned int */ - unsigned int iseed; + uint64 iseed; = if (seed =3D=3D NULL || strcmp(seed, "time") =3D=3D 0) { @@ -5139,7 +5158,7 @@ set_random_seed(const char *seed) instr_time now; = INSTR_TIME_SET_CURRENT(now); - iseed =3D (unsigned int) INSTR_TIME_GET_MICROSEC(now); + iseed =3D (uint64) INSTR_TIME_GET_MICROSEC(now); } else if (strcmp(seed, "rand") =3D=3D 0) { @@ -5155,7 +5174,7 @@ set_random_seed(const char *seed) /* parse seed unsigned int value */ char garbage; = - if (sscanf(seed, "%u%c", &iseed, &garbage) !=3D 1) + if (sscanf(seed, UINT64_FORMAT "%c", &iseed, &garbage) !=3D 1) { fprintf(stderr, "unrecognized random seed option \"%s\": expecting an unsigned integ= er, \"time\" or \"rand\"\n", @@ -5165,10 +5184,14 @@ set_random_seed(const char *seed) } = if (seed !=3D NULL) - fprintf(stderr, "setting random seed to %u\n", iseed); - srandom(iseed); - /* no precision loss: 32 bit unsigned int cast to 64 bit int */ + fprintf(stderr, "setting random seed to " UINT64_FORMAT "\n", iseed); random_seed =3D iseed; + + /* Fill base_random_sequence with low-order bits of seed */ + base_random_sequence.xseed[0] =3D iseed & 0xFFFF; + base_random_sequence.xseed[1] =3D (iseed >> 16) & 0xFFFF; + base_random_sequence.xseed[2] =3D (iseed >> 32) & 0xFFFF; + return true; } = @@ -5862,10 +5885,9 @@ main(int argc, char **argv) /* set default seed for hash functions */ if (lookupVariable(&state[0], "default_seed") =3D=3D NULL) { - uint64 seed =3D ((uint64) (random() & 0xFFFF) << 48) | - ((uint64) (random() & 0xFFFF) << 32) | - ((uint64) (random() & 0xFFFF) << 16) | - (uint64) (random() & 0xFFFF); + uint64 seed =3D + ((uint64) getrand(&base_random_sequence, 0, 0xFFFFFFFF)) | + (((uint64) getrand(&base_random_sequence, 0, 0xFFFFFFFF)) << 32); = for (i =3D 0; i < nclients; i++) if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed= )) ------- =_aaaaaaaaaa0--