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

[Tatsuo Ishii]

> In general looks good to me, however, maybe rename prng_state to
> something like backsel_state, or something similar. That way it's
> more clear that it's a dedicated state for backend selection. Else
> it would be just as easy to use pg_prng.c's pg_global_prng_state
> variable. But that's just my colour bikeshed.

I think renaming prng_state is a good idea. In v3 patch I changed the
variable name to backsel_state as you suggested. Also I slightly
modified the commit message, and added "Author" field. I think now the
patch is commit-able shape.

If there's no objection, I will commit the patch.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

> On Thu, 2025-05-29 at 10:33 +0900, Tatsuo Ishii wrote:
>> > > 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: v3-0001-Replace-random-with-pg_prng-random-function.patch
Type: application/octet-stream
Size: 20495 bytes
Desc: not available
URL: <http://www.pgpool.net/pipermail/pgpool-hackers/attachments/20250605/acf1020a/attachment-0001.obj>