From: Tom Lane <tgl@sss.pgh.pa.us>
To: Fabien COELHO <coelho@cri.ensmp.fr>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD
Date: Tue, 22 Jan 2019 11:44:57 -0500
Message-ID: <9718.1548175497@sss.pgh.pa.us> (raw)
In-Reply-To: <alpine.DEB.2.21.1901202225310.28945@lancre>
References: <9427.1547701450@sss.pgh.pa.us>
<alpine.DEB.2.21.1901170659210.7416@lancre>
<21538.1547770868@sss.pgh.pa.us>
<4615.1547792324@sss.pgh.pa.us>
<alpine.DEB.2.21.1901180926580.26418@lancre>
<29225.1547834182@sss.pgh.pa.us>
<alpine.DEB.2.21.1901182257480.20734@lancre>
<alpine.DEB.2.21.1901201051270.14950@lancre>
<7879.1548015981@sss.pgh.pa.us>
<alpine.DEB.2.21.1901202225310.28945@lancre>
Fabien COELHO <coelho@cri.ensmp.fr> 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
Attachments:
[text/x-diff] dont-use-srandom-in-pgbench-1.patch (4.4K, ../9718.1548175497@sss.pgh.pa.us/2-dont-use-srandom-in-pgbench-1.patch)
download | inline diff:diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.cindex b5c75ce..d5b543f 100644--- a/src/bin/pgbench/pgbench.c+++ b/src/bin/pgbench/pgbench.c@@ -185,7 +185,7 @@ int64 latency_limit = 0;
char *tablespace = NULL;
char *index_tablespace = NULL;
-/* random seed used when calling srandom() */+/* random seed used to initialize base_random_sequence */
int64 random_seed = -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[] =
/* 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 *dv)
/*
* Initialize a random state struct.
+ *+ * We derive the seed from base_random_sequence, which must be set up already.
*/
static void
initRandomState(RandomState *random_state)
{
- random_state->xseed[0] = random();- random_state->xseed[1] = random();- random_state->xseed[2] = random();+ random_state->xseed[0] = (unsigned short)+ getrand(&base_random_sequence, 0, 0xFFFF);+ random_state->xseed[1] = (unsigned short)+ getrand(&base_random_sequence, 0, 0xFFFF);+ random_state->xseed[2] = (unsigned short)+ getrand(&base_random_sequence, 0, 0xFFFF);
}
-/* random number generator: uniform distribution from min to max inclusive */+/*+ * Random number generator: uniform distribution from min to max inclusive.+ *+ * Although the limits are expressed as int64, you can't generate the full+ * 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, instr_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 sequences.+ */
static bool
set_random_seed(const char *seed)
{
- /* srandom expects an unsigned int */- unsigned int iseed;+ uint64 iseed;
if (seed == NULL || strcmp(seed, "time") == 0)
{
@@ -5139,7 +5158,7 @@ set_random_seed(const char *seed)
instr_time now;
INSTR_TIME_SET_CURRENT(now);
- iseed = (unsigned int) INSTR_TIME_GET_MICROSEC(now);+ iseed = (uint64) INSTR_TIME_GET_MICROSEC(now);
}
else if (strcmp(seed, "rand") == 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) != 1)+ if (sscanf(seed, UINT64_FORMAT "%c", &iseed, &garbage) != 1)
{
fprintf(stderr,
"unrecognized random seed option \"%s\": expecting an unsigned integer, \"time\" or \"rand\"\n",
@@ -5165,10 +5184,14 @@ set_random_seed(const char *seed)
}
if (seed != 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 = iseed;
++ /* Fill base_random_sequence with low-order bits of seed */+ base_random_sequence.xseed[0] = iseed & 0xFFFF;+ base_random_sequence.xseed[1] = (iseed >> 16) & 0xFFFF;+ base_random_sequence.xseed[2] = (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") == NULL)
{
- uint64 seed = ((uint64) (random() & 0xFFFF) << 48) |- ((uint64) (random() & 0xFFFF) << 32) |- ((uint64) (random() & 0xFFFF) << 16) |- (uint64) (random() & 0xFFFF);+ uint64 seed =+ ((uint64) getrand(&base_random_sequence, 0, 0xFFFFFFFF)) |+ (((uint64) getrand(&base_random_sequence, 0, 0xFFFFFFFF)) << 32);
for (i = 0; i < nclients; i++)
if (!putVariableInt(&state[i], "startup", "default_seed", (int64) seed))
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: tgl@sss.pgh.pa.us, coelho@cri.ensmp.fr, pgsql-hackers@lists.postgresql.org
Subject: Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD
In-Reply-To: <9718.1548175497@sss.pgh.pa.us>
* 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