Thread: BUGFIX: Dynamic bgworkers with BGW_NEVER_RESTART worker restarted on FatalError
BUGFIX: Dynamic bgworkers with BGW_NEVER_RESTART worker restarted on FatalError
From
Craig Ringer
Date:
Hi all There's a bug in the dynamic bgworkers code that I think needs fixing before release. TL;DR: BGW_NO_RESTART workers are restarted after postmaster crash, attached patch changes that. The case that's triggering the issue is where a static bgworker is registering a new dynamic bgworker to do some setup work each time it starts. The static worker is relaunched on postmaster restart and registers a new BGW_NO_RESTART dynamic bgworker. This dynamic bgworker immediately hits an Assert and dies. This *should* cause a postmaster restart loop; that's expected. What it shouldn't do, but is doing, is restart multiple copies of the bgworker - fail to purge the old BGW_NO_RESTART one and launch it as if it'd never crashed. The attached patch fixes the problem. Detail: If you set a worker as BGW_NO_RESTART it isn't restarted if it ERRORs out. That's fine. With Petr's applied patch it no longer restarts on exit 0 (normal exit) either. There's a third case, though: a bgworker crash causing postmaster restart. In this case the bgworker is still restarted, which makes no sense at all if it isn't for the other two cases. The existing code looks like it tries to protect against this - in maybe_start_bgworker, the invocation of do_start_bgworker is protected by a prior test for rw->rw_crashed_at that, if rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART, unregisters the worker and skips to the next one. However, in my testing a breakpoint inside the if (rw->rw_crashed_at != 0) test in maybe_start_bgworker is never hit, even for a bgworker that is known to have crashed. rw->rw_crashed_at is always zero. The culprit is ResetBackgroundWorkerCrashTimes, which unconditionally resets the crash time without considering that the worker might be BGW_NO_RESTART. The attached patch makes ResetBackgroundWorkerCrashTimes only reset the crashed time for workers with a restart time set. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Re: BUGFIX: Dynamic bgworkers with BGW_NEVER_RESTART worker restarted on FatalError
From
Robert Haas
Date:
On Fri, May 16, 2014 at 1:38 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > There's a bug in the dynamic bgworkers code that I think needs fixing > before release. TL;DR: BGW_NO_RESTART workers are restarted after > postmaster crash, attached patch changes that. > > The case that's triggering the issue is where a static bgworker is > registering a new dynamic bgworker to do some setup work each time it > starts. The static worker is relaunched on postmaster restart and > registers a new BGW_NO_RESTART dynamic bgworker. This dynamic bgworker > immediately hits an Assert and dies. > > This *should* cause a postmaster restart loop; that's expected. What it > shouldn't do, but is doing, is restart multiple copies of the bgworker - > fail to purge the old BGW_NO_RESTART one and launch it as if it'd never > crashed. > > The attached patch fixes the problem. Hmm, I think Petr's original patch might have even done something like this. It did have some sort of conditional around setting rw_crashed_at; I can't remember if it's the same as what you've got here. But I changed it, because I said to myself something along the lines: "Surely it can't hurt to reset rw_crashed_at unconditionally." Evidently, it can. But let's think about this scenario. Suppose some backend (it doesn't matter if it is a user session or another background worker) launches a run-once (i.e. BGW_NEVER_RESTART) background worker. Before the postmaster actually starts that worker, it seg faults or fails an assertion, and the postmaster does a crash-and-restart. After resetting, that background worker will still have rw_crashed_at = 0, and will get started. Is that what we want? I'd argue probably not. If you agree, perhaps we should treat all background workers as having been started at least once and then crashed, but a long time ago so that if they're supposed to be restarted that happens right away. In other words, have ResetBackgroundWorkerCrashTimes() unconditionally fill in a non-zero but very old value (say, 1) rather than zero. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company