Date: Wed, 11 Dec 2024 19:34:14 -0800
From: Noah Misch <noah@leadboat.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Fix early elog(FATAL)
Message-ID: <20241212033414.e0.nmisch@google.com>
References: <20241208034614.45.nmisch@google.com>
 <Z1i-K95hTcggVOgn@nathan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Z1i-K95hTcggVOgn@nathan>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241212033414.e0.nmisch%40google.com>
Precedence: bulk

On Tue, Dec 10, 2024 at 04:18:19PM -0600, Nathan Bossart wrote:
> On Sat, Dec 07, 2024 at 07:46:14PM -0800, Noah Misch wrote:
> > Three ways to fix this:
> 
> I noticed that you committed a fix for this.  Sorry for not responding
> earlier.
> 
> > 1. Call InitProcessGlobals() earlier.  This could also reduce the total call
> >    sites from 3 to 2 (main() and post-fork).
> > 
> > 2. Move MyProcPid init out of InitProcessGlobals(), to main() and post-fork.
> >    This has less to go wrong in back branches.  While probably irrelevant,
> >    this avoids calling pg_prng_strong_seed() in processes that will exit after
> >    help() or GucInfoMain().
> > 
> > 3. Revert 97550c0, as commit 3b00fdb anticipated.
> 
> I did partially revert 97550c0 in commit 8fd0498, but we decided to leave
> some of the checks

Got it.  Old branches couldn't merely do (3), anyway, since 3b00fdb is v17+.
I missed that while writing the list.

> > I don't think the choice matters much, so here is (2).
> 
> FWIW I'd probably vote for option 1.  That keeps the initialization of the
> globals together, reduces the call sites, and fixes the bug.  I'd worry a
> little about moving the MyProcPid assignments out of that function without
> adding a bunch of commentary to explain why.

Can you say more about that?  A comment about MyProcPid could say "fork() is
the one thing that changes the getpid() return value".  To me, the things
InitProcessGlobals() sets are all different.  MyProcPid can be set without
elog(ERROR) and gets invalidated at fork().  The others reasonably could
elog(ERROR).  (They currently don't.)  The random state could have a different
lifecycle.  If we had a builtin pooler that reused processes, we'd
reinitialize random state at each process reuse, not at each fork().  So I see
the grouping of (MyProcPid, MyStartTimestamp, random seed) as mostly an
accident of history.

Thanks,
nm