Re: Re: [PATCH] Clear FatalError earlier during crash restart - Mailing list pgsql-hackers
| From | Trakshan Mishra |
|---|---|
| Subject | Re: Re: [PATCH] Clear FatalError earlier during crash restart |
| Date | |
| Msg-id | CACRpqq9Ta8Umv36YAEKXJ+kixyjAzg8X2kRz2=GT=NS9J83fOQ@mail.gmail.com Whole thread |
| In response to | Re: [PATCH] Clear FatalError earlier during crash restart (Ayush Tiwari <ayushtiwari.slg01@gmail.com>) |
| List | pgsql-hackers |
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
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:
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
pgsql-hackers by date: