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