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.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 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.pl
index 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',
