From: Nathan Bossart <nathandbossart@gmail.com>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Fix early elog(FATAL)
Date: Fri, 13 Dec 2024 22:14:43 -0600
Message-ID: <Z10GM5BBojDI3u14@nathan> (raw)
In-Reply-To: <20241214031505.05.nmisch@google.com>
References: <20241208034614.45.nmisch@google.com>
<Z1i-K95hTcggVOgn@nathan>
<20241212033414.e0.nmisch@google.com>
<Z1sKJBE6-Lo08PEt@nathan>
<20241214031505.05.nmisch@google.com>
On Fri, Dec 13, 2024 at 07:15:05PM -0800, Noah Misch wrote:
> 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.
I just noticed that InitProcessGlobals() is relatively new. It was added
in v12 by commit 197e4af.
>> 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.
I don't feel strongly either way, either. I don't think it's important
enough to diverge from the back-branches.
--
nathan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, noah@leadboat.com
Subject: Re: Fix early elog(FATAL)
In-Reply-To: <Z10GM5BBojDI3u14@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox