Re: [PATCH] Clear FatalError earlier during crash restart

On Fri, Sep 25, 2026 at 01:44:26AM +0530, Ayush Tiwari wrote:
> Then I remembered Noah's suggestion [1] in Justin's older thread on this
> same hang [2]: clear FatalError when we relaunch startup. The attached v1
> tries that instead, with a small TAP test. AFAICS, the old children are
> gone and shmem is rebuilt by then. FWIW, if the new checkpointer crashes
> in that window, we now do a full cleanup instead of silently respawning
> it. Does this look like the right point to clear the flag?

Hmm. In the case where the children cannot be forked like in the
WIN32 scenario that we got back then in ead8f696b7cd, then setting
FatalError to false as you do, aka earlier means that we may do the
reinit work of the 3232-3273 block more than necessary (the
"FatalError && pmState == PM_NO_CHILDREN" things)..

RemovePgTempFiles() stands out easily as the most expensive piece, but
this also implies a continuous rebuild of shared memory. It looks
like that to me:
- Reinit block runs, sets FatalError = false.
- Child dies (cannot be forked because of WIN32 bleah, could be a
checkpointer, a bgworker..), process_pm_child_exit() cleans it, calls
HandleChildCrash().
- FatalError is false, so we finish by calling HandleFatalError()
. HandleFatalError() sets FatalError = true, again, calls
TerminateChildren() which would kill the startup process.
- The children are gone, the reinit block fires again, FatalError is
back to false.

That would mean an oscillation of FatalError to be
false->true->false->true, implying that for each process that cannot
be forked we would just do what could be an annoying reinit. Now my
question would be, echoing with yours: is that really worth worrying
about? Perhaps no. Your patch transforms a step that was basically
free with the checkpointer respawning to something that could be much
more expensive, and we gain from that more stability in the shutdown
sequence.

Honestly I am not sure if this is an improvement, because it also
means that with your patch the postmaster gives up depending on which
child is cleaned up first. On HEAD we have a more deterministic
choice, as far as I understand: if the startup process is down, we are
down.

> One wrinkle is #19623 [3]. The special case from ead8f696b7c becomes
> unreachable with this, so the patch removes it and puts back the
> Assert(!FatalError) in HandleFatalError(). IIUC, if every new child
> fails, we could now reap another child's exit before the startup
> process's and go for another restart instead of giving up. Michael, do
> you think that case still needs special handling? I haven't tested that
> scenario.

I have done again the test mentioned in ead8f696b7cd based on the
trick of making InitPostmasterChild() fail aggressively, and the
postmaster does not get stuck.

To me, it sounds like one root issue points to FatalError meaning two
things at once:
- crash handling is in progress, don't re-enter the reinit loop.
- we drain the children after a crash, like the btmask_add() in
PostmasterStateMachine(), or maybe_start_io_workers_scheduled_at()
where we shortcut the startups.

But perhaps we would not gain much in making the tracking of these
states more complicated. On the contrary, in the postmaster, simpler
is better, and simpler means that less states is probably better
long-term because it makes the signaling and shutdown sequences easier
to think about.
--
Michael