Re: Dependency between bgw_notify_pid and bgw_flags

In CleanupBackgroundWorker(), we seem to differentiate between a background
worker with shared memory access and a backend.

2914 /*
2915 * Additionally, for shared-memory-connected workers, just
like a
2916 * backend, any exit status other than 0 or 1 is considered a
crash
2917 * and causes a system-wide restart.
2918 */

There is an assertion on line 2943 which implies that a backend should have
a database connection.

2939
2940 /* Get it out of the BackendList and clear out remaining data
*/
2941 if (rw->rw_backend)
2942 {
2943 Assert(rw->rw_worker.bgw_flags &
BGWORKER_BACKEND_DATABASE_CONNECTION);

A backend is a process created to handle a client connection [1], which is
always connected to a database. After custom background workers were
introduced we seem to have continued that notion. Hence only the background
workers which request database connection are in BackendList. So, may be we
should continue with that notion and correct the comment as "Background
workers that request database connection during registration are in this
list, too." or altogether delete that comment.

With that notion of backend, to fix the original problem I reported,
PostmasterMarkPIDForWorkerNotify() should also look at the
BackgroundWorkerList. As per the comments in the prologue of this function,
it assumes that only backends need to be notified about worker state
change, which looks too restrictive. Any backend or background worker,
which wants to be notified about a background worker it requested to be
spawned, should be allowed to do so.

5655 /*
5656 * When a backend asks to be notified about worker state changes, we
5657 * set a flag in its backend entry. The background worker machinery
needs
5658 * to know when such backends exit.
5659 */
5660 bool
5661 PostmasterMarkPIDForWorkerNotify(int pid)

PFA the patch which changes PostmasterMarkPIDForWorkerNotify to look at
BackgroundWorkerList as well.

The backends that request to be notified are marked using bgworker_notify,
so that when such a backend dies the notifications to it can be turned off
using BackgroundWorkerStopNotifications(). Now that we allow a background
worker to be notified, we have to build similar flag in RegisteredBgWorker
for the same purpose. The patch does that.
[1]. http://www.postgresql.org/developer/backend/

On Mon, Jun 8, 2015 at 11:22 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jun 8, 2015 at 1:51 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
> > Robert Haas wrote:
> >
> >> After studying this, I think it's a bug. See this comment:
> >>
> >> * Normal child backends can only be launched when we are in PM_RUN or
> >> * PM_HOT_STANDBY state. (We also allow launch of normal
> >> * child backends in PM_WAIT_BACKUP state, but only for superusers.)
> >> * In other states we handle connection requests by launching "dead_end"
> >> * child processes, which will simply send the client an error message
> and
> >> * quit. (We track these in the BackendList so that we can know when
> they
> >> * are all gone; this is important because they're still connected to
> shared
> >> * memory, and would interfere with an attempt to destroy the shmem
> segment,
> >> * possibly leading to SHMALL failure when we try to make a new one.)
> >> * In PM_WAIT_DEAD_END state we are waiting for all the dead_end
> children
> >> * to drain out of the system, and therefore stop accepting connection
> >> * requests at all until the last existing child has quit (which
> hopefully
> >> * will not be very long).
> >>
> >> That comment seems to imply that, at the very least, all backends with
> >> shared-memory access need to be part of BackendList. But really, I'm
> >> unclear why we'd ever want to exit without all background workers
> >> having shut down, so maybe they should all be in there. Isn't that
> >> sorta the point of this facility?
> >>
> >> Alvaro, any thoughts?
> >
> > As I recall, my thinking was:
> >
> > * anything connected to shmem that crashes could leave things in bad
> > state (for example locks improperly held), whereas things not connected
> > to shmem could crash without it being a problem for the rest of the
> > system and thus do not require a full restart cycle. This stuff is
> > detected with the PMChildSlot thingy; therefore all bgworkers with shmem
> > access should have one of these.
> >
> > * I don't recall offhand what the criteria is for stuff to be in
> > postmaster's BackendList. According to the comment on top of struct
> > Backend, bgworkers connected to shmem should be on that list, even if
> > they did not have the BGWORKER_BACKEND_DATABASE_CONNECTION flag on
> > registration. So that would be a bug.
> >
> > Does this help you any?
>
> Well, it sounds like we at least need to think about making it
> consistently depend on shmem-access rather than database-connection.
> I'm less sure what to do with workers that have not even got shmem
> access.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company