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
