pgsql: Set random seed for pgbench.

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

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.

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

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.

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

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