Re: Re: [HACKERS] pgbench randomness initialization - Mailing list pgsql-hackers
From | Chapman Flack |
---|---|
Subject | Re: Re: [HACKERS] pgbench randomness initialization |
Date | |
Msg-id | 5A54847E.5050801@anastigmatix.net Whole thread Raw |
In response to | Re: [HACKERS] pgbench randomness initialization (Fabien COELHO <coelho@cri.ensmp.fr>) |
Responses |
Re: pgbench randomness initialization
Re: Re: [HACKERS] pgbench randomness initialization Re: Re: [HACKERS] pgbench randomness initialization |
List | pgsql-hackers |
On 01/02/18 05:57, Fabien COELHO wrote: >> Here is a new version which output use used seed when a seed is >> explicitely set with an option or from the environment. This is a simple patch that does what it says on the tin. I ran into trouble with the pgbench TAP test *even before applying the patch*, but only because I was doing a VPATH build as a user without 'write' on the source tree (001_pgbench_with_server.pl tried to make pgbench create log files there). Bad me. Oddly, that was the only test in the whole tree to have such an issue, so here I add a pre-patch to fix that. Now my review needs a review. :) With that taken care of, the added tests all pass for me. I wonder, though: > The tests assume that stdlib random/srandom behavior is standard thus > deterministic between platform. Is the behavior of srandom() and the system generator really so precisely specified that seed 5432 will produce the same values hardcoded in the tests on all platforms? It does on mine, but could it produce spurious test failures for others? I guess the (or a) spec would be here: http://pubs.opengroup.org/onlinepubs/7908799/xsh/initstate.html It specifies a "non-linear additive feedback random-number generator employing a default state array size of 31 long integers", but it does not pin down parameters or claim only one candidate exists. To have the test run pgbench twice with the same seed and compare the results sounds safer. This revised pgbench-seed-4.patch changes the code not at all, and the TAP test only in whitespace. I did some wordsmithing of the doc, which I hope was not presumptuous of me, only as a conversation starter. I expanded the second sentence because on my first reading I wasn't quite sure of its meaning. Once I had looked at the code, I could see that the sentence was economical and precise already, but I tried to find a version that would also have been clear to the me-before-I-looked. The documentation doesn't say that --random-seed= (or PGBENCH_RANDOM_SEED=) will have the same effect as 'time', and indeed, I really think it should be unset (defaulting to 'time'), or 'time', or 'rand', or an integer, or an error. The error, right now, says only "expecting an unsigned integer"; it should also mention time and rand. Should 'rand' be something that conveys more about its meaning, 'strong' perhaps? The documentation doesn't mention the allowed range of the unsigned integer (which I get, because 'unsigned int' is exactly the signature for srandom, but somebody might read "unsigned integer" in a more generic sense). I'm not sure what would be a better way to say it. The base (only decimal, as now in the code) isn't specified either. Maybe the documentation should mention that the output now includes the random seed being used, so that (even without planning ahead) a session can be re-run with the same seed if necessary. It could just say "an unsigned integer in decimal, as it is shown in pgbench's output" or something like that. Something more may need to be said in the doc about reproducibility. I think the real story is that a run can be reproduced if the number of clients is equal to the number of threads. Although each thread has its own generator state, each client does not (if there is more than one per thread), and as soon as real select() delays start happening in CSTATE_WAIT_RESULT, the clients dealt out to a given thread may not be hitting that thread's generator in a deterministic order. -Chap
Attachment
pgsql-hackers by date: