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

[Tatsuo Ishii]

>> 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.

Okay. I changed select_load_balancing_node() so that it has its own
prng state data and initialization. Fo the prng initialization I added
initialize_prng() in the same file, and remove the prng initialization
from do_child().

>> - 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 */

Thanks. I did it in initialize_prng();

>>   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.

Agreed. The complexity is not worth the trouble.

Please see attached v2 patch.

> 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 --------------
A non-text attachment was scrubbed...
Name: v2-0001-Replace-random-with-pg_prng-random-function.patch
Type: application/octet-stream
Size: 20313 bytes
Desc: not available
URL: <http://www.pgpool.net/pipermail/pgpool-hackers/attachments/20250529/7802171a/attachment-0001.obj>