Re: make MaxBackends available in _PG_init

Julien Rouhaud <rjuju123@gmail.com>

From: Julien Rouhaud <rjuju123@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Michael Paquier <michael@paquier.xyz>, "Bossart, Nathan" <bossartn@amazon.com>, Fujii Masao <masao.fujii@oss.nttdata.com>, "wangsh.fnst@fujitsu.com" <wangsh.fnst@fujitsu.com>, Andres Freund <andres@anarazel.de>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Greg Sabino Mullane <htamfids@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2022-03-25T03:11:46Z
Lists: pgsql-hackers

Attachments

On Fri, Mar 25, 2022 at 10:39:51AM +0800, Julien Rouhaud wrote:
> On Thu, Mar 24, 2022 at 04:27:36PM -0400, Robert Haas wrote:
> > On Thu, Mar 24, 2022 at 4:20 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > > Another possibility could be to add a hook that is called _before_
> > > _PG_init() where libraries are permitted to adjust GUCs.  After the library
> > > is loaded, we first call this _PG_change_GUCs() function, then we
> > > initialize MaxBackends, and then we finally call _PG_init().  This way,
> > > extensions would have access to MaxBackends within _PG_init(), and if an
> > > extension really needed to alter GUCs, іt could define this new function.
> > 
> > Yeah, I think this might be better.
> 
> Well, if it's before _PG_init() then it won't be a new hook but a new symbol
> that has to be handled with dlsym.
> 
> But it seems to be that the bigger problem is that this approach won't fix
> anything unless we can prevent third-party code from messing with GUCs after
> that point as we could otherwise have discrepancies between GetMaxBackends()
> and the underlying GUCs until every single extension that wants to change GUC
> is modified uses this new symbol.  And I also don't see how we could force that
> unless we have a better GUC API that doesn't entirely relies on strings,
> otherwise a simple thing like "allow 5 extra bgworkers" is going to be really
> painful.
> 
> A new hook after _PG_init() sure leaves the burden to the few extension that
> needs to allocate shmem based on MaxBackends, but there probably isn't that
> much (I have a few dozens locally cloned and found only 1 apart from mine, and
> I would be happy to take care of those), and it also means that they have a
> chance to do something correct even if other extensions messing with GUCs
> aren't fixed.
> 
> Arguably, you could certainly try to change GUCs in that new hook, but it
> wouldn't be any different from doing so in shmem_startup_hook so I don't think
> it's really a problem.

As an example, here's a POC for a new shmem_request_hook hook after _PG_init().
With it I could easily fix pg_wait_sampling shmem allocation (and checked that
it's indeed requesting the correct size).

Commits

  1. Add a new shmem_request_hook hook.

  2. Remove non-functional code for unloading loadable modules.

  3. Fix misleading comments about background worker registration.

  4. Revert the addition of GetMaxBackends() and related stuff.

  5. Fix typo in multixact.c

  6. Reduce more the number of calls to GetMaxBackends()

  7. Remove MaxBackends variable in favor of GetMaxBackends() function.

  8. Fix comments about bgworker registration before MaxBackends initialization

  9. Fix bogus assertion in BootstrapModeMain().

  10. Make sure MaxBackends is always set