Re: [PATCH] Clear FatalError earlier during crash restart

On Thu, Sep 24, 2026 at 8:14 PM UTC Ayush Tiwari <
ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> I ran into a smart/fast shutdown hang on master while looking at crash
> restart.

I tested v1 on master 16a104073d0 (your stated base 46024c573bc is 12
commits back; it still applies cleanly with git am). Build was meson,
--buildtype=debug -Dcassert=true, zero warnings.

First, confirming the bug and the fix. I built a test-only tree (your
058 plus the meson.build hunk, postmaster.c left alone) and ran it
against unpatched master:

recovery/058_shutdown_crash_restart FAIL after 181.04s

The server log shows "received fast shutdown request" and then nothing
at all for three minutes, until the harness forced an immediate
shutdown. With the full patch the same test passes in 0.60s, and 20
consecutive runs were clean. The whole recovery suite is green on the
patched tree: 57 tests, 50 ok / 7 skipped / 0 fail.

Smart shutdown hangs in that window too, and your patch fixes it as
well. Measured, entering the window and then requesting each mode:

patched/smart shut down OK in 1s unpatched/smart hung (>30s)
patched/fast shut down OK in 0s unpatched/fast hung (>30s)

Since the commit message claims both, it would be worth covering smart
in the test as well; parameterising 058 over ('fast','smart') is cheap.

> Do you think that case still needs special handling?

No, and I could not produce the restart loop you were worried about.

I reproduced the #19623 shape directly: enter crash restart, hold the
relaunched startup process in restore_command, then SIGKILL it before
redo starts.

patched: postmaster exited, "reinitializing" appeared once
"startup process (PID n) was terminated by signal 9"
"shutting down due to startup process failure"

unpatched: postmaster exited, "reinitializing" appeared once
... plus "aborting startup due to startup process failure"

Same outcome; master just logs one extra line. The reason there is no
loop is that PostmasterStateMachine() tests StartupStatus ==
STARTUP_CRASHED at postmaster.c:3214 and calls ExitPostmaster(1)
*before* the "if (FatalError && pmState == PM_NO_CHILDREN)" reinit
block at :3232. That gate does the real work; ead8f696b7c's shortcut
was only needed because HandleChildCrash() used to return early with
FatalError already set.

The StartupStatus assignment survives your new path, which is the part
I wanted to be sure of: StartupPMChild is set to NULL at :2280 before
HandleChildCrash() runs, so the TerminateChildren() inside
HandleFatalError() does not overwrite STARTUP_CRASHED with
STARTUP_SIGNALED.

Non-startup children crashing in the same window behave sensibly too.
There StartupPMChild is non-NULL, so TerminateChildren() does set
STARTUP_SIGNALED, and the startup process dying on the SIGQUIT routes
to PM_WAIT_BACKENDS -- one more restart cycle rather than an exit.

I also checked the restored assertion. All three HandleFatalError()
callers are guarded against FatalError already being set: :2835 by the
early return at :2824, :3090 by the else branch of "if (Shutdown >=
ImmediateShutdown || FatalError)", and :3898 by "else if (!FatalError
&& ...)". So Assert(!FatalError) looks safe to put back.

> Does this look like the right point to clear the flag?

The point is right, the placement within it is a few lines late. You
clear FatalError at :3268, but UpdatePMState(PM_STARTUP) is at :3259,
so :3259-3267 is a window where pmState is PM_STARTUP and FatalError is
still true -- covering maybe_start_io_workers() and StartChildProcess().
That is also why the comment you rewrote at :330-333 isn't quite right
yet: it drops the old "It can be true in PM_STARTUP state" sentence
while the code still permits exactly that.

Moving the two assignments above UpdatePMState(PM_STARTUP) makes the
documented invariant literally true, and it is behaviour-neutral:
maybe_start_io_workers() tests "FatalError && pmState >=
PM_STOP_BACKENDS", and PM_STARTUP sorts before PM_STOP_BACKENDS, so it
takes the same branch either way; StartChildProcess() never reads
FatalError. I tested that reordering -- clean build, 058 passes, the
#19623 scenario still exits with one "reinitializing", recovery suite
unchanged.

ShmemCallRequestCallbacks();
CreateSharedMemoryAndSemaphores();

+ /* The old children are gone; proceed as at initial startup */
+ FatalError = false;
+ AbortStartTime = 0;
+
UpdatePMState(PM_STARTUP);

/* Make sure we can perform I/O while starting up. */
@@
StartupPMChild = StartChildProcess(B_STARTUP);
Assert(StartupPMChild != NULL);
StartupStatus = STARTUP_RUNNING;
- /* The old children are gone; proceed as at initial startup */
- FatalError = false;
- AbortStartTime = 0;

On the tests, three things.

The #19623 path now has no coverage on either side. ead8f696b7c shipped
no test of its own (one file, postmaster.c). Your patch removes it and
leans on the STARTUP_CRASHED gate instead, so that gate is load-bearing
and untested. Given that removing a committed fix is the most
contentious part of this patch, locking the behaviour down with a test
would help it along. Roughly: wait for the helper's message, pull the
startup PID out of the log (log_line_prefix gives you "startup[pid]"),
SIGKILL it, then assert the postmaster exits by itself and the log has
"shutting down due to startup process failure". One wrinkle to solve:
the restore_command helper is a child of the startup process and
outlives the kill, so it needs bounding or reaping, otherwise it lingers
holding the log fd.

In 058 the helper's budget and the stop timeout are both
$PostgreSQL::Test::Utils::timeout_default, so in the failure case they
expire together and whether "timed out waiting for shutdown request"
reaches the log before stop() gives up is a race. Giving the helper a
larger budget than the stop timeout makes that second assertion
deterministic.

Otherwise the test reads well, and it is a real regression test rather
than a vacuous one -- the server log shows it genuinely entering the
window (reinitializing, new startup process, blocked in
restore_command). wait_for_shutdown follows the cp_history_files
precedent exactly, same file mode and the same "$perlbin"
"$FindBin::RealBin/..." invocation.

One last thing worth a line in the commit message: clearing FatalError
earlier means BgWorkerStart_PostmasterStart workers can now start during
crash-restart PM_STARTUP, where maybe_start_bgworkers()'s "if
(FatalError)" used to hold them back until redo began. That matches
initial startup, which is what you are going for, and the exposure is
small since those workers cannot request a database connection. But it
is a user-visible change for extensions, and no in-tree module uses that
start time, so no test would catch it.

Regards,
Trakshan Mishra

On Fri, Sep 25, 2026 01:45 AM, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com>
wrote:

> Hi,
>
> I ran into a smart/fast shutdown hang on master while looking at crash
> restart. It happens in the window after we relaunch startup, but before
> WAL redo starts. FatalError is still set, so we wait for the new
> checkpointer and I/O workers after sending SIGTERM, which they ignore.
> AbortStartTime has already been reset too, so nothing escalates to
> SIGKILL.
>
> I first tried routing that shutdown through
> HandleFatalError(PMQUIT_FOR_STOP, false), i.e. sending SIGQUIT to the
> children. That fixed the hang, but still went down the abnormal-shutdown
> path and left the stale flag in place.
>
> 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?
>
> 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've added a WIP TAP test too which may need modifications.
> (I haven't tried the patch on the back branches yet.)
>
> Regards,
> Ayush
>
> [1] https://www.postgresql.org/message-id/20241025181701.ed.
> nmisch(at)google(dot)com
> [2] https://www.postgresql.org/message-id/flat/
> ZWlrdQarrZvLsgIk(at)pryzbyj2023
> (Thomas's patch there is CF 4884)
> [3] https://www.postgresql.org/message-id/flat/19623-
> f9bd331940be1273(at)postgresql(dot)org
>