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

[Tatsuo Ishii]

I have pushed the patch to master branch. Thank you!
https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=1a68c6eb0b2afb470485a4d8ed5b7cb494e2e442

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

From: Martijn van Duren <pgpool at list.imperialat.at>
Subject: Re: [pgpool-hackers: 4598] Re: Shuffle random functions and use better random numbers
Date: Thu, 05 Jun 2025 14:22:42 +0200
Message-ID: <809b54c47304eccf5f96faa190805df3fc56df40.camel at list.imperialat.at>

> From just a quick glance, and changes are as stated, no objections from
> me.
> 
> On Thu, 2025-06-05 at 19:07 +0900, Tatsuo Ishii wrote:
>> > 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
>> > > > > 
>> > 
>