Re: Re: [PATCH] Clear FatalError earlier during crash restart

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(dot)slg01(at)gmail(dot)com>
wrote:

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