Process initialization labyrinth
Hi,
While working on the shared memory stats patch I (not for the first
time), issues with our process initialization.
The concrete issue was that I noticed that some stats early in startup
weren't processed correctly - the stats system wasn't initialized yet. I
consequently added assertions ensuring that we don't try to report stats
before that. Which blew up.
Even in master we report stats well before the pgstat_initialize()
call. E.g. in autovac workers:
/*
* Report autovac startup to the stats collector. We deliberately do
* this before InitPostgres, so that the last_autovac_time will get
* updated even if the connection attempt fails. This is to prevent
* autovac from getting "stuck" repeatedly selecting an unopenable
* database, rather than making any progress on stuff it can connect
* to.
*/
That previously just didn't cause a problem, because we didn't really
need pgstat_initialize() to have happened for stats reporting to work.
In the shared memory stats patch there's no dependency on
pgstat_initialize() knowing MyBackendId anymore (broken out to a
separate function). So I tried moving the stats initialization to
somewhere earlier.
There currently is simply no way of doing that that doesn't cause
duplication, or weird conditions. We can't do it in:
- InitProcess()/InitAuxiliaryProcess(),
CreateSharedMemoryAndSemaphores() hasn't yet run in EXEC_BACKEND
- below CreateSharedMemoryAndSemaphores(), as that isn't called for each
backend in !EXEC_BACKEND
- InitPostgres(), because autovac workers report stats before that
- BaseInit(), because it's called before we have a PROC iff !EXEC_BACKEND
- ...
I have now worked around this by generous application of ugly, but I
think we really need to do something about this mazy mess. There's just
an insane amount of duplication, and it's too complicated to remember
more than a few minutes.
I really would like to not see things like
/*
* Create a per-backend PGPROC struct in shared memory, except in the
* EXEC_BACKEND case where this was done in SubPostmasterMain. We must do
* this before we can use LWLocks (and in the EXEC_BACKEND case we already
* had to do some stuff with LWLocks).
*/
#ifdef EXEC_BACKEND
if (!IsUnderPostmaster)
InitProcess();
#else
InitProcess();
#endif
Similarly, codeflow like bootstrap.c being involved in bog standard
stuff like starting up wal writer etc, is just pointlessly
confusing. Note that bootstrap itself does *not* go through
AuxiliaryProcessMain(), and thus has yet another set of initialization
needs.
Greetings,
Andres Freund
On Thu, Apr 1, 2021 at 8:22 PM Andres Freund <andres@anarazel.de> wrote:
<snip>
I have now worked around this by generous application of ugly, but I
think we really need to do something about this mazy mess. There's just
an insane amount of duplication, and it's too complicated to remember
more than a few minutes.I really would like to not see things like
/*
* Create a per-backend PGPROC struct in shared memory, except in the
* EXEC_BACKEND case where this was done in SubPostmasterMain. We must do
* this before we can use LWLocks (and in the EXEC_BACKEND case we already
* had to do some stuff with LWLocks).
*/
#ifdef EXEC_BACKEND
if (!IsUnderPostmaster)
InitProcess();
#else
InitProcess();
#endifSimilarly, codeflow like bootstrap.c being involved in bog standard
stuff like starting up wal writer etc, is just pointlessly
confusing. Note that bootstrap itself does *not* go through
AuxiliaryProcessMain(), and thus has yet another set of initialization
needs.
I can't really speak to the initial points directly relating to
pgstat/shmem, but for the overall maze-like nature of the startup
code: is there any chance the startup centralization patchset would be
of any help here?
/messages/by-id/CAMN686FE0OdZKp9YPO=htC6LnA6aW4r-+jq=3Q5RAoFQgW8EtA@mail.gmail.com
I know you are at least vaguely aware of the efforts there, as you
reviewed the patchset. Figured I should at least bring it up in case
it seemed helpful, or an effort you'd like to re-invigorate.
Thanks,
--
Mike Palmiotto
https://crunchydata.com
Hi,
On 2021-04-02 10:18:20 -0400, Mike Palmiotto wrote:
I can't really speak to the initial points directly relating to
pgstat/shmem, but for the overall maze-like nature of the startup
code: is there any chance the startup centralization patchset would be
of any help here?
/messages/by-id/CAMN686FE0OdZKp9YPO=htC6LnA6aW4r-+jq=3Q5RAoFQgW8EtA@mail.gmail.com
I think parts of it could help, at least. It doesn't really do much
about centralizing / de-mazing the actual initialization of sub-systems,
but it'd make it a bit easier to do so.
Greetings,
Andres Freund
On Fri, Apr 2, 2021 at 1:02 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2021-04-02 10:18:20 -0400, Mike Palmiotto wrote:
I can't really speak to the initial points directly relating to
pgstat/shmem, but for the overall maze-like nature of the startup
code: is there any chance the startup centralization patchset would be
of any help here?
/messages/by-id/CAMN686FE0OdZKp9YPO=htC6LnA6aW4r-+jq=3Q5RAoFQgW8EtA@mail.gmail.comI think parts of it could help, at least. It doesn't really do much
about centralizing / de-mazing the actual initialization of sub-systems,
but it'd make it a bit easier to do so.
The patchset needs some work, no doubt. If you think it'd be useful, I
can spend some of my free time addressing any gaps in the design. I'd
hate to see that code go to waste, as I think it may have been a
reasonable first step.
Also not opposed to you taking the patchset and running with it if you prefer.
Thanks,
--
Mike Palmiotto
https://crunchydata.com