Date: Fri, 13 Dec 2024 19:15:05 -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: <20241214031505.05.nmisch@google.com>
References: <20241208034614.45.nmisch@google.com>
 <Z1i-K95hTcggVOgn@nathan>
 <20241212033414.e0.nmisch@google.com>
 <Z1sKJBE6-Lo08PEt@nathan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Z1sKJBE6-Lo08PEt@nathan>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241214031505.05.nmisch%40google.com>
Precedence: bulk

On Thu, Dec 12, 2024 at 10:07:00AM -0600, Nathan Bossart wrote:
> On Wed, Dec 11, 2024 at 07:34:14PM -0800, Noah Misch wrote:
> > On Tue, Dec 10, 2024 at 04:18:19PM -0600, Nathan Bossart wrote:
> >> 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.
> 
> Fair enough.  I suppose part of my hesitation stems from expecting hackers
> to be more likely to remember to call InitProcessGlobals() than to
> initialize MyProcPid.  But given your change requires initializing
> MyProcPid in exactly 2 places, and there are unlikely to be more in the
> near future, I might be overthinking it.

I don't feel strongly either way.  I did write it the option-1 way originally.
Then I started thinking about changes at a distance causing the other
InitProcessGlobals() tasks to palloc or elog.  We could do option-1 in master
and keep the back branches in their current state.