[PATCH] Clear FatalError earlier during crash restart

On Thu, Sep 24, 2026 at 8:14 PM UTC Ayush Tiwari <
ayushtiwari.slg01@gmail.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.slg01@gmail.com>
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.

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

Hi,

On Wed, 30 Sept 2026 at 05:32, Michael Paquier <michael@paquier.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]--- 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(); 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);
+ }
  }

On Wed, Sep 30, 2026 at 05:44:16AM +0530, Ayush Tiwari wrote:

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.

Yeah, I can see that, due to this part I think because the FatalError
flag would be set? Quoting the relevant part from postmaster.c:
if (Shutdown > NoShutdown && pmState == PM_NO_CHILDREN)
{
if (FatalError)
{
ereport(LOG, (errmsg("abnormal database system shutdown")));
ExitPostmaster(1);
}
else
{
/*
* Normal exit from the postmaster is here. We don't need to log
* anything here, since the UnlinkLockFiles proc_exit callback
* will do so, and that should be the last user-visible action.
*/
ExitPostmaster(0);
}
}

So that just feels, err, okay-ish, because the logs reflect what we
are actually doing?

So plugging in an extra HandleFatalError() while the pmState is
PM_STOP_BACKENDS feels like a good compromise, on top of my mind. It
would mean that your too-early-shutdown request on crash recovery
would still persist on v17 and older branches, but at least the
checkpointer and the I/O workers would be able to understand that they
need to stop, and the SIGQUIT sent would be promoted to a SIGKILL
after the AbortStartTime timeout is armed.

I was slightly on the edge about your TAP test and its fancy restore
command, but I don't want to leave that untested, either, as that
seems like the same thing as Justin's case. Perhaps only do that on
HEAD first, let it brew for a bit, and consider it down later on if
really necessary? These shutdown changes stress me quite a bit when
it comes to a backpatch, because they can be nasty very easily with
one mistake, and I like a peaceful sleep. On top of that v19 is close
by.
--
Michael

Hi,

On Wed, 30 Sept 2026 at 06:32, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Sep 30, 2026 at 05:44:16AM +0530, Ayush Tiwari wrote:

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.

Yeah, I can see that, due to this part I think because the FatalError
flag would be set? Quoting the relevant part from postmaster.c:
if (Shutdown > NoShutdown && pmState == PM_NO_CHILDREN)
{
if (FatalError)
{
ereport(LOG, (errmsg("abnormal database system shutdown")));
ExitPostmaster(1);
}
else
{
/*
* Normal exit from the postmaster is here. We don't need to log
* anything here, since the UnlinkLockFiles proc_exit callback
* will do so, and that should be the last user-visible action.
*/
ExitPostmaster(0);
}
}

So that just feels, err, okay-ish, because the logs reflect what we
are actually doing?

So plugging in an extra HandleFatalError() while the pmState is
PM_STOP_BACKENDS feels like a good compromise, on top of my mind. It
would mean that your too-early-shutdown request on crash recovery
would still persist on v17 and older branches, but at least the
checkpointer and the I/O workers would be able to understand that they
need to stop, and the SIGQUIT sent would be promoted to a SIGKILL
after the AbortStartTime timeout is armed.

Thanks, I've gone back to the smaller fix in v2 (attached), leaving the
FatalError reset where it was.

I was slightly on the edge about your TAP test and its fancy restore
command, but I don't want to leave that untested, either [...]

I've kept the helper, but now wait for the new checkpointer to install its
signal handlers. The helper gets more time too.

Perhaps only do that on HEAD first, let it brew for a bit, and consider
it down later on if really necessary?

You have a better feel for the backpatch risk here, so I'll defer to you..

Regards,
Ayush

Hi Ayush,

I tested v2 on master (9510a826e4a). It applies cleanly and builds
without warnings (meson, debug, cassert).

- 058_shutdown_crash_restart passes on the patched tree: 20 separate
runs, all passed, about 1.2s each.
- With only the test applied (postmaster.c as on master) it fails as it
should: pg_ctl reports "server does not shut down" after the 180s
timeout.
- The test only covers fast shutdown. Changing it locally to
stop('smart') also passes 5/5 with v2, and smart hangs on master the
same way, so a second stop() case may be worth adding.
- Full recovery suite on the patched tree: 50 ok, 7 skipped, 0 failed.

The server log shows what Michael expected: the new HandleFatalError()
path (PM_STOP_BACKENDS -> PM_WAIT_BACKENDS), then "abnormal database
system shutdown" about 0.2s after the request. The startup process also
logs a FATAL "could not restore file ... terminated by signal 3", since
the SIGQUIT reaches the restore_command child too.

One small thing: the test waits for "checkpointer updated shared memory
configuration values", an elog(DEBUG2) in checkpointer.c. If that
wording changes, the test would time out for an unrelated reason.

I haven't tested the back branches.

Regards,
Trakshan

On Wed, Sep 30, 2026 07:53 AM, Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

On Wed, Sep 30, 2026 at 07:53:12AM +0530, Ayush Tiwari wrote:

I've kept the helper, but now wait for the new checkpointer to install its
signal handlers. The helper gets more time too.

Hmm. I am not sure that we actually need to multiply by 4 the restore
timeout sent to the fake restore_command waiting for the shutdown.
Isn't that somewhat betraying the default value of Utils.pm?

You have a better feel for the backpatch risk here, so I'll defer to you..

HEAD-only feels like better sleep for the time being woth v19 being
close by. Better sleep sounds always good.

I have been doing a bunch of adjustments as of the attached, and that
seems to work properly. The buildfarm would be the final judge; the
CI looks OK with the new test at least. What do you think?
--
Michael

Hi,

On Thu, 1 Oct 2026 at 09:48, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Sep 30, 2026 at 07:53:12AM +0530, Ayush Tiwari wrote:

I've kept the helper, but now wait for the new checkpointer to install its
signal handlers. The helper gets more time too.

Hmm. I am not sure that we actually need to multiply by 4 the restore
timeout sent to the fake restore_command waiting for the shutdown.
Isn't that somewhat betraying the default value of Utils.pm?

Ahh the 4x was just headroom for the setup waits.

You have a better feel for the backpatch risk here, so I'll defer to you..

HEAD-only feels like better sleep for the time being woth v19 being
close by. Better sleep sounds always good.

Sounds good.

I have been doing a bunch of adjustments as of the attached, and that
seems to work properly. The buildfarm would be the final judge; the
CI looks OK with the new test at least. What do you think?

Thanks for this. The patch looks good to me.
I did multiple A/B testing and it works well.

Regards,
Ayush

On Thu, Oct 01, 2026 at 10:54:55AM +0530, Ayush Tiwari wrote:

Thanks for this. The patch looks good to me.
I did multiple A/B testing and it works well.

I've been going back and forth a bit in the postmaster code to try
more if FatalError can be made again non-re-entrant for
HandleFatalError() with an assertion. My first try was the
introduction of a new flag to track if SIGQUIT was sent, but I was not
completely sure that I got the semantics right. A second set of
thoughts was pointing towards a more complicated scheme for the
states, which felt not really exciting. At the end, I have just
reused your solution, and applied it on HEAD. Let's let it brew first
there. The lack of complaints on the matter (even if we had one back
in 2023), and the v19 release being very close by don't argue in favor
of a backpatch, at least it feels strongly so to me.

Now down to the buildfarm to judge the stability of your test. The CI
has cleared quite a few times.
--
Michael

Hi,

Some data on the backpatch question, since the lack of reports was
mentioned.

I hit this hang outside of a test this week, on a server based on 18.6.
A backend crashed with SIGSEGV and my script ran "pg_ctl stop -m fast"
right after it. The stop never finished. The machine was busy, so the
restarted startup process was still syncing the data directory (about
3 seconds) when the shutdown request came in. The postmaster then kept
waiting for the checkpointer and the IO workers, as described upthread.
Only "-m immediate" brought it down.

It reproduces without a restore_command, with only a crash and the
timing. The attached script creates a cluster with about 9000 relation
files, so that the sync takes a few seconds, kills a backend with
SIGSEGV, and runs "pg_ctl stop -m fast" as soon as "reinitializing"
shows up in the log. What I got:

REL_17_STABLE (35c508af520) hangs (checkpointer)
REL_18_STABLE (3c6135f839d) hangs (checkpointer, IO workers)
REL_19_STABLE (28f836dacaf) hangs (checkpointer, IO workers)
master before 12f866cd5f7 (a32b8dc9865) hangs
master (f58ab18a904) stops in about a second
REL_18_STABLE + 12f866cd5f7 stops; 058 passes, and fails
without the fix
REL_19_STABLE + 12f866cd5f7 stops; 058 passes

12f866cd5f7 applies cleanly to REL_18_STABLE. On REL_19_STABLE the only
conflict is the test list in src/test/recovery/meson.build, where 059
is already listed.

I understand wanting it to settle on HEAD first. I mainly wanted to say
that it happens outside of tests: anything that stops the server right
after a backend crash can land in that window, and on a busy machine
the window is a few seconds long. Since 19 is not released yet, it may
be worth considering for it.

Thanks for the fix.

Regards,
Muzzammil Sarwar

On Wed, Oct 7, 2026 at 5:07 AM Michael Paquier <michael@paquier.xyz> wrote: