Re: [PATCH] Clear FatalError earlier during crash restart

Hi,

On Wed, 30 Sept 2026 at 05:32, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

Ah, sorry, I hadn't given the repeated reinit work much thought, to be
honest. Removing temp files and rebuilding shmem whenever another child
fails could be quite a drag.

> > 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.

Thanks for checking. It's good to know it doesn't wedge in that test..

> 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.

I think that argues for the smaller fix, at least on the back branches.
I'd tried the diff below[1] before clearing FatalError: it SIGQUITs the new
children on smart/fast shutdown, without changing the crash-restart
behavior. It fixed my repro on 46024c573bc, though it logs an abnormal
shutdown.

Thanks again for explaining to me the drawbacks.

Regards,
Ayush

[1]
---
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3041,9 +3041,14 @@ PostmasterStateMachine(void)
*/
ForgetUnstartedBackgroundWorkers();

- SignalChildren(SIGTERM, targetMask);
+ if (FatalError)
+ HandleFatalError(PMQUIT_FOR_STOP, false);
+ else
+ {
+ SignalChildren(SIGTERM, targetMask);

- UpdatePMState(PM_WAIT_BACKENDS);
+ UpdatePMState(PM_WAIT_BACKENDS);
+ }
}