Aleksander Alekseev <aleksan...@timescale.com> writes:
> It looks like the patch is in pretty good shape. I noticed that the
> return value of pg_prng_strong_seed() is not checked in several
> places, also there was a typo in pg_trgm.c. The corrected patch is
> attached. Assuming the new version will not upset cfbot, I would call
> the patch "Ready for Committer".
I took a quick look through this. The biggest substantive point
I found was that you didn't update the configure script. It's
certainly not appropriate for configure to continue to do
AC_REPLACE_FUNCS on random and srandom when you've removed the
src/port files that that would attempt to include.
The simplest change here is just to delete those entries from the
list, but that would also result in not #define'ing HAVE_RANDOM
or HAVE_SRANDOM, and I see that the patch introduces a dependency
on the latter. I'm inclined to think that's misguided. srandom()
has been required by POSIX since SUSv2, and we certainly have not
got any non-Windows buildfarm members that lack it. So I don't
think we really need a configure check. What we do need is a decision
about what to do on Windows. We could write it like
+#ifndef WIN32
+ srandom(pg_prng_i32(&pg_global_prng_state));
+#endif
but I have a different modest suggestion: add
#define srandom(seed) srand(seed)
in win32_port.h. As far as I can see from Microsoft's docs [1],
srand() is exactly like srandom(), they just had some compulsion
to not be POSIX-compatible.
BTW, the commentary in InitProcessGlobals is now completely
inadequate; it's unclear to a reader why we should be bothering
ith srandom(). I suggest adding a comment right before the
srandom() call, along the lines of
/*
* Also make sure that we've set a good seed for random() (or rand()
* on Windows). Use of those functions is deprecated in core
* Postgres, but they might get used by extensions.
*/
+/* use Donald Knuth's LCG constants for default state */
How did Knuth get into this? This algorithm is certainly not his,
so why are those constants at all relevant?
Other cosmetic/commentary issues:
* I could do without the stream-of-consciousness notes in pg_prng.c.
I think what's appropriate is to say "We use thus-and-such a generator
which is documented here", maybe with a line or two about its properties.
* Function names like these convey practically nothing to readers:
+extern int64 pg_prng_i64(pg_prng_state *state);
+extern uint32 pg_prng_u32(pg_prng_state *state);
+extern int32 pg_prng_i32(pg_prng_state *state);
+extern double pg_prng_f64(pg_prng_state *state);
+extern bool pg_prng_bool(pg_prng_state *state);
and these functions' header comments add a grand total of zero bits
of information. What someone generally wants to know first about
a PRNG is (a) is it uniform and (b) what is the range of outputs,
neither of which are specified anywhere.
+#define FIRST_BIT_MASK UINT64CONST(0x8000000000000000)
+#define RIGHT_HALF_MASK UINT64CONST(0x00000000FFFFFFFF)
+#define DMANTISSA_MASK UINT64CONST(0x000FFFFFFFFFFFFF)
I'm not sure that these named constants are any more readable than
writing the underlying constant, maybe less so --- in particular
I think something based on (1<<52)-1 would be more appropriate for
the float mantissa operations. We don't need RIGHT_HALF_MASK at
all, the casts to uint32 or int32 will accomplish that just fine.
BTW, why are we bothering with FIRST_BIT_MASK in the first place,
rather than returning "v & 1" for pg_prng_bool? Is xoroshiro128ss
less random in the low-order bits than the higher? If so, that would
be a pretty important thing to document. If it's not, we shouldn't
make the code look like it is.
+ * select in a range with bitmask rejection.
What is "bitmask rejection"? Is it actually important to callers?
I think this should be documented more like "Produce a random
integer uniformly selected from the range [rmin, rmax)."
regards, tom lane
[1]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/srand?view=msvc-170
