Обсуждение: BUGFIX: Dynamic bgworkers with BGW_NEVER_RESTART worker restarted on FatalError

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

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