pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Set random seed for pgbench.
7+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2018-03-31 5635c7aa67 Fix portability and translatability issues in commit 64f85894a.
* pgsql: Set random seed for pgbench.
@ 2018-03-26 15:26 Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Teodor Sigaev @ 2018-03-26 15:26 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Set random seed for pgbench.
Setting random could increase reproducibility of test in some cases. Patch
suggests three providers for seed: time (default), strong random
generator (if available) and unsigned constant. Seed could be set from
command line or enviroment variable.
Author: Fabien Coelho
Reviewed by: Chapman Flack
Discussion: https://www.postgresql.org/message-id/flat/20160407082711.q7iq3ykffqxcszkv@alap3.anarazel.de
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/64f85894ad2730fb1449a8e81dd8026604e9a546
Modified Files
--------------
doc/src/sgml/ref/pgbench.sgml | 42 +++++++++++++++++
src/bin/pgbench/pgbench.c | 69 +++++++++++++++++++++++++---
src/bin/pgbench/t/001_pgbench_with_server.pl | 69 +++++++++++++++++++++++++---
src/bin/pgbench/t/002_pgbench_no_server.pl | 2 +
4 files changed, 170 insertions(+), 12 deletions(-)
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
@ 2018-03-31 02:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2018-03-31 02:48 UTC (permalink / raw)
To: Teodor Sigaev <teodor@sigaev.ru>; +Cc: pgsql-committers@lists.postgresql.org, Fabien COELHO <coelho@cri.ensmp.fr>
Teodor Sigaev <teodor@sigaev.ru> writes:
> Set random seed for pgbench.
So this patch has ignored the possibility of not having pg_strong_random.
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?
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.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-03-31 07:00 ` Fabien COELHO <coelho@cri.ensmp.fr>
2018-03-31 16:34 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Fabien COELHO @ 2018-03-31 07:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Teodor Sigaev <teodor@sigaev.ru>; pgsql-committers@lists.postgresql.org
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) download
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',
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
@ 2018-03-31 16:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 17:43 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2018-03-31 16:34 UTC (permalink / raw)
To: Fabien COELHO <coelho@cri.ensmp.fr>; +Cc: Teodor Sigaev <teodor@sigaev.ru>; pgsql-committers@lists.postgresql.org
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> So this patch has ignored the possibility of not having pg_strong_random.
> I assumed that pg_strong_random is always available,
... which is wrong. Every other call of it is wrapped in
#ifdef HAVE_STRONG_RANDOM, and so must this one be.
We can use the same error message though, I suppose.
Adjusted and pushed.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
2018-03-31 16:34 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-03-31 17:43 ` Fabien COELHO <coelho@cri.ensmp.fr>
2018-04-01 07:22 ` Re: pgsql: Set random seed for pgbench. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 7+ messages in thread
From: Fabien COELHO @ 2018-03-31 17:43 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Teodor Sigaev <teodor@sigaev.ru>; pgsql-committers@lists.postgresql.org
>> I assumed that pg_strong_random is always available,
>
> ... which is wrong. Every other call of it is wrapped in
> #ifdef HAVE_STRONG_RANDOM, and so must this one be.
Indeed, I clearly misunderstood its usage pattern. I looked at its source
("src/port/pg_strong_random.c") where the function is always defined and
is documented as returning false if it does not find a strong random
source, so I though that checking for this was enough. But indeed its
compilation fails if no source is provided.
Thanks for the fix.
--
Fabien.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
2018-03-31 16:34 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 17:43 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
@ 2018-04-01 07:22 ` Michael Paquier <michael@paquier.xyz>
2018-04-01 16:13 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Michael Paquier @ 2018-04-01 07:22 UTC (permalink / raw)
To: Fabien COELHO <coelho@cri.ensmp.fr>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Teodor Sigaev <teodor@sigaev.ru>; pgsql-committers@lists.postgresql.org
On Sat, Mar 31, 2018 at 07:43:38PM +0200, Fabien COELHO wrote:
> Indeed, I clearly misunderstood its usage pattern. I looked at its source
> ("src/port/pg_strong_random.c") where the function is always defined and is
> documented as returning false if it does not find a strong random source, so
> I though that checking for this was enough. But indeed its
> compilation fails if no source is provided.
I have not check in details this thread so I may be saying something
stupid... But if you are looking for a frontend implementation for
strong randoms, please extract pg_frontend_random in fe-auth-scram.c and
move it to its own file for example in src/common as a frontend-only
file. When working on SCRAM, I recall mentioning that but Heikki has
kept the code in its current shape for simplicity.
As far as I can see, there is no reason to issue an error in
set_random_seed() either in pgbench code.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Set random seed for pgbench.
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
2018-03-31 16:34 ` Re: pgsql: Set random seed for pgbench. Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 17:43 ` Re: pgsql: Set random seed for pgbench. Fabien COELHO <coelho@cri.ensmp.fr>
2018-04-01 07:22 ` Re: pgsql: Set random seed for pgbench. Michael Paquier <michael@paquier.xyz>
@ 2018-04-01 16:13 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 7+ messages in thread
From: Tom Lane @ 2018-04-01 16:13 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Fabien COELHO <coelho@cri.ensmp.fr>; Teodor Sigaev <teodor@sigaev.ru>; pgsql-committers@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> I have not check in details this thread so I may be saying something
> stupid... But if you are looking for a frontend implementation for
> strong randoms, please extract pg_frontend_random in fe-auth-scram.c and
> move it to its own file for example in src/common as a frontend-only
> file. When working on SCRAM, I recall mentioning that but Heikki has
> kept the code in its current shape for simplicity.
I don't really see the point of doing more work than we've done there.
No modern platform should be without pg_strong_random, and if you've
actually had to use --disable-strong-random, it will not be astonishing
that you lose some functionality.
Also, while I personally do not see a use-case why somebody would need
cryptographically strong random seeds in pgbench, if somebody did need
that then they wouldn't thank us for silently falling back to a
not-strong generation method if the platform is misconfigured somehow.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2018-04-01 16:13 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-03-26 15:26 pgsql: Set random seed for pgbench. Teodor Sigaev <teodor@sigaev.ru>
2018-03-31 02:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 07:00 ` Fabien COELHO <coelho@cri.ensmp.fr>
2018-03-31 16:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-03-31 17:43 ` Fabien COELHO <coelho@cri.ensmp.fr>
2018-04-01 07:22 ` Michael Paquier <michael@paquier.xyz>
2018-04-01 16:13 ` Tom Lane <tgl@sss.pgh.pa.us>
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