Re: [PATCH] random_normal function

Hello Paul,

My 0.02€ about the patch:

Patch did not apply with "git apply", I had to "patch -p1 <" and there was
a bunch of warnings.

Patches compile and make check is okay.

The first patch adds empty lines at the end of "sql/random.sql", I think
that it should not.

# About random_normal:

I'm fine with providing a random_normal implementation at prng and SQL
levels.

There is already such an implementation in "pgbench.c", which outputs
integers, I'd suggest that it should also use the new prng function, there
should not be two box-muller transformations in pg.

# About pg_prng_double_normal:

On the comment, I'd write "mean + stddev * val" instead of starting with
the stddev part.

Usually there is an empty line between the variable declarations and the
first statement.

There should be a comment about why it needs u1
larger than some epsilon. This constraint seems to generate a small bias?

I'd suggest to add the UNLIKELY() compiler hint on the loop.

# About random_string:

Should the doc about random_string tell that the output bytes are expected
to be uniformly distributed? Does it return "random values" or "pseudo
random values"?

I do not understand why the "drandom_string" function is in "float.c", as
it is not really related to floats. Also it does not return a string but a
bytea, so why call it "_string" in the first place? I'm do not think that
it should use pg_strong_random which may be very costly on some platform.
Also, pg_strong_random does not use the seed, so I do not understand why
it needs to be checked. I'd suggest that the output should really be
uniform pseudo-random, possibly based on the drandom() state, or maybe
not.

Overall, I think that there should be a clearer discussion and plan about
which random functionS postgres should provide to complement the standard
instead of going there… randomly:-)

--
Fabien.