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

[Martijn van Duren]

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.

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