From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Teodor Sigaev <teodor@sigaev.ru>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Set random seed for pgbench.
Date: Sat, 31 Mar 2018 09:00:02 +0200 (CEST)
Message-ID: <alpine.DEB.2.20.1803310828100.23311@lancre> (raw)
In-Reply-To: <32390.1522464534@sss.pgh.pa.us>
References: <E1f0U1H-00005J-98@gemulon.postgresql.org>
<32390.1522464534@sss.pgh.pa.us>
Hello Tom,
>> Set random seed for pgbench.
>
> So this patch has ignored the possibility of not having pg_strong_random.
I assumed that pg_strong_random is always available, but may fail with an
error if a strong random source is not available, in which case the error
message should be enough for the user to figure out that they cannot use
the 'rand' value and must rely on 'time' or an integer.
The error message can be improved in this case.
> That's moving the portability goalposts in a way that I do not find
> acceptable for such a marginal feature. What behavior do you think we
> should have for platforms without that --- accept the seed=rand option
> but then error out, or not recognize the option at all?
The first option is currently implemented and documented, and seems fine.
> BTW, I also note that the patch is constructing error messages from
> sentence fragments, which is verboten per translatability policy.
> While that's perhaps not too important as long as we don't have message
> translations for pgbench, it still seems like something to fix now not
> later.
The elements of the structure could be translatable independently: one
part says there is an error, the other provides the context in which this
error was triggered. This could be made a little clearer by using
parentheses for instance. Otherwise to comply with translability policy
the function can be made to return a bool and then the caller show the
context.
In the attached, I improved the error message on 'rand' error and printed
the error context outside of the function.
Thanks for the review.
--
Fabien.
Attachments:pgbench-seed-fix-1.patch (text/plain, 3.3K) downloaddiff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.cindex 8529e7d..8aa462b 100644--- a/src/bin/pgbench/pgbench.c+++ b/src/bin/pgbench/pgbench.c@@ -4669,8 +4669,8 @@ printResults(TState *threads, StatsData *total, instr_time total_time,
}
/* call srandom based on some seed. NULL triggers the default behavior. */
-static void-set_random_seed(const char *seed, const char *origin)+static bool+set_random_seed(const char *seed)
{
/* srandom expects an unsigned int */
unsigned int iseed;
@@ -4687,8 +4687,10 @@ set_random_seed(const char *seed, const char *origin)
/* use some "strong" random source */
if (!pg_strong_random(&iseed, sizeof(iseed)))
{
- fprintf(stderr, "cannot seed random from a strong source\n");- exit(1);+ fprintf(stderr,+ "cannot seed random from a strong source, none available: "+ "use 'time' or an unsigned integer value.\n");+ return false;
}
}
else
@@ -4698,9 +4700,9 @@ set_random_seed(const char *seed, const char *origin)
if (sscanf(seed, "%u%c", &iseed, &garbage) != 1)
{
fprintf(stderr,
- "error while scanning '%s' from %s, expecting an unsigned integer, 'time' or 'rand'\n",- seed, origin);- exit(1);+ "error while scanning '%s', expecting an unsigned integer, 'time' or 'rand'\n",+ seed);+ return false;
}
}
@@ -4709,6 +4711,7 @@ set_random_seed(const char *seed, const char *origin)
srandom(iseed);
/* no precision loss: 32 bit unsigned int cast to 64 bit int */
random_seed = iseed;
+ return true;
}
@@ -4823,7 +4826,11 @@ main(int argc, char **argv)
memset(state, 0, sizeof(CState));
/* set random seed early, because it may be used while parsing scripts. */
- set_random_seed(getenv("PGBENCH_RANDOM_SEED"), "PGBENCH_RANDOM_SEED environment variable");+ if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))+ {+ fprintf(stderr, "error while setting random seed from PGBENCH_RANDOM_SEED environment variable");+ exit(1);+ }
while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1)
{
@@ -5099,7 +5106,11 @@ main(int argc, char **argv)
break;
case 9: /* random-seed */
benchmarking_option_set = true;
- set_random_seed(optarg, "--random-seed option");+ if (!set_random_seed(optarg))+ {+ fprintf(stderr, "error while setting random seed from --random-seed option");+ exit(1);+ }
break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.plindex 682bc22..8a3c4be 100644--- a/src/bin/pgbench/t/002_pgbench_no_server.pl+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl@@ -111,7 +111,8 @@ my @options = (
[qr{unrecognized initialization step},
qr{allowed steps are} ] ],
[ 'bad random seed', '--random-seed=one',
- [qr{error while scanning 'one' from --random-seed option, expecting an unsigned integer} ] ],+ [qr{error while scanning 'one', expecting an unsigned integer},+ qr{error while setting random seed from --random-seed option} ] ],
# loging sub-options
[ 'sampling => log', '--sampling-rate=0.01',
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-committers@postgresql.org
Cc: coelho@cri.ensmp.fr, tgl@sss.pgh.pa.us, teodor@sigaev.ru, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Set random seed for pgbench.
In-Reply-To: <alpine.DEB.2.20.1803310828100.23311@lancre>
* 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