[pgpool-hackers: 4598] Re: Shuffle random functions and use better random numbers

[Tatsuo Ishii]

[Re-send with Cc: to pgpool-hackers]

> Looks like you missed pg_prng.h in your diff.

Oops. pg_prng.h attached.

> No strong objections, but some questions:
> - Why put initialize_prng() inside do_child()? Wouldn't it make sense
>   to put it inside select_load_balancing_node() under a
>   `static int init` guard? That way people might not overlook to
>   initialize it if the prng code is used somewhere else, in another
>   process.
> - I'm not too fond of initializing the entire prng_state with random
>   crap. While it currently only holds two uint64 values that are
>   supposed to be random, it could cause major head scratching if with
>   some update adds a new non-random field to it. If nothing else it
>   just feels like bad practice. I'd suggest to something like
>   uint64 seed;
>   if (!pg_strong_random(&seed, sizeof(seed)))
>     seed = 1; /* Pick a value, as long as it spreads */
>   pg_prng_seed(&pg_global_prng_state, seed);
> - initialize_prng feels overengineered. If strong crypto fails why not
>   fall back to simply a value of 1, or simply crash? The goal is to
>   have an even spread over the backends, not that the distribution
>   follows a different path on each restart.

I will answer them later on.

> 
> For the rest no issues spotted from reading the code, but since
> pg_prng.h is missing I can't compile/run test it.
>
> Martijn van Duren
> 
> On Wed, 2025-05-21 at 15:58 +0900, Tatsuo Ishii wrote:
>> > I have pushed the patch to master branch, with slightly modified
>> > commit message, and credited you as a reviewer.
>> 
>> Attached is the followup commit to import pg_prng.c and use
>> pg_prng_double() for calculating a random number between 0 and 1.0.
>> Actually pg_prng_double() generates a random number [0, 1.0) (1.0 is
>> not inclusive), I think it's ok for our purpose.
>> 
>> [ from the commit message]
>> 
>> Other notes regarding the port:
>> 
>> - pg_prng needs to be initialized using pg_prng_strong_seed() per
>>   process. Currently the only caller is child.c (per session
>>   process). If other process needs to use pg_prng, it needs the same
>>   initialization as child.c.
>> 
>> - Some of functions in the file were not ported because they require
>>   additional library: pg_bitutils.c. In the future we may revisit and
>>   import pg_bitutils.c.
>> 
>> - likely/unlikely are ignored. In the future we may revisit and import
>>   them.
>> 
>> - All conditional compiling regarding "sun" or "_sun" are removed. It
>>   seems the platform is not used for running pgpool anymore.
>> 
>> - Since srandom() is not necessary any more, related code are removed
>>   from pgpool_main.c, child.c and pcp_worker.c.
>> 
>> Best regards,
>> --
>> Tatsuo Ishii
>> SRA OSS K.K.
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp
> 
-------------- next part --------------
/*-------------------------------------------------------------------------
 *
 * Pseudo-Random Number Generator
 *
 * Copyright (c) 2021-2025, PostgreSQL Global Development Group
 *
 * src/include/common/pg_prng.h
 *
 *-------------------------------------------------------------------------
 */
#ifndef PG_PRNG_H
#define PG_PRNG_H

/*
 * State vector for PRNG generation.  Callers should treat this as an
 * opaque typedef, but we expose its definition to allow it to be
 * embedded in other structs.
 */
typedef struct pg_prng_state
{
	uint64		s0,
				s1;
} pg_prng_state;

/*
 * Callers not needing local PRNG series may use this global state vector,
 * after initializing it with one of the pg_prng_...seed functions.
 */
extern PGDLLIMPORT pg_prng_state pg_global_prng_state;

extern void pg_prng_seed(pg_prng_state *state, uint64 seed);
extern void pg_prng_fseed(pg_prng_state *state, double fseed);
extern bool pg_prng_seed_check(pg_prng_state *state);

/*
 * Initialize the PRNG state from the pg_strong_random source,
 * taking care that we don't produce all-zeroes.  If this returns false,
 * caller should initialize the PRNG state from some other random seed,
 * using pg_prng_[f]seed.
 *
 * We implement this as a macro, so that the pg_strong_random() call is
 * in the caller.  If it were in pg_prng.c, programs using pg_prng.c
 * but not needing strong seeding would nonetheless be forced to pull in
 * pg_strong_random.c and thence OpenSSL.
 */
#define pg_prng_strong_seed(state) \
	(pg_strong_random(state, sizeof(pg_prng_state)) ? \
	 pg_prng_seed_check(state) : false)

extern uint64 pg_prng_uint64(pg_prng_state *state);
extern uint64 pg_prng_uint64_range(pg_prng_state *state, uint64 rmin, uint64 rmax);
extern int64 pg_prng_int64(pg_prng_state *state);
extern int64 pg_prng_int64p(pg_prng_state *state);
extern int64 pg_prng_int64_range(pg_prng_state *state, int64 rmin, int64 rmax);
extern uint32 pg_prng_uint32(pg_prng_state *state);
extern int32 pg_prng_int32(pg_prng_state *state);
extern int32 pg_prng_int32p(pg_prng_state *state);
extern double pg_prng_double(pg_prng_state *state);
extern double pg_prng_double_normal(pg_prng_state *state);
extern bool pg_prng_bool(pg_prng_state *state);

#endif							/* PG_PRNG_H */