random() (was Re: New GUC to sample log queries)

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Thanks!  I pushed this with two changes -- one was to reword the docs a
> bit more, and the other was to compute in_sample only if it's going to
> be used (when exceeded is true).  I hope this won't upset any compilers ...
> I wonder if there's any sensible way to verify the behavior in an
> automated fashion.  Who know what marvels we'd discover about the
> reliability/uniformity of random() across platforms.

So Coverity doesn't like this patch:

2250     		in_sample = exceeded &&
2251     			log_statement_sample_rate != 0 &&
2252     			(log_statement_sample_rate == 1 ||
>>>     CID 1441909:  Security best practices violations  (DC.WEAK_CRYPTO)
>>>     "random" should not be used for security related applications, as linear congruential algorithms are too easy to break.
2253     			 random() <= log_statement_sample_rate * MAX_RANDOM_VALUE);
2254     
2255     		if ((exceeded && in_sample) || log_duration)

I am not sure I buy the argument that this is a security hazard, but
there are other reasons to question the use of random() here, some of
which you stated yourself above.  Another one is that using random()
for internal purposes interferes with applications' possible use of
drandom() and setseed(), ie an application depending on getting a
particular random series would see different behavior depending on
whether this GUC is active or not.

I wonder whether we should establish a project policy to avoid use
of random() for internal purposes, ie try to get to a point where
drandom() is the only caller in the backend.  A quick grep says
that there's a dozen or so callers, so this patch certainly isn't
the only offender ... but should we make an effort to convert them
all to use, say, pg_erand48()?  I think all the existing callers
could happily share a process-wide random state, so we could make
a wrapper that's no harder to use than random().

Another idea, which would be a lot less prone to breakage by
add-on code, is to change drandom() and setseed() to themselves
use pg_erand48() with a private seed.  Then we could positively
promise that their behavior isn't affected by anything else
(and it'd become platform-independent, too, which it probably
isn't today).  There would be a one-time compatibility breakage
for anyone who's depending on the exact current behavior, but
that might be OK considering how weak our guarantees are for it
right now.

			regards, tom lane