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 <robertmh...@gmail.com> wrote:
> On Mon, Jun 8, 2015 at 1:51 PM, Alvaro Herrera <alvhe...@2ndquadrant.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
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index f57224c..c49b97b 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -332,20 +332,21 @@ BackgroundWorkerStateChange(void)
rw->rw_worker.bgw_notify_pid = 0;
}
/* Initialize postmaster bookkeeping. */
rw->rw_backend = NULL;
rw->rw_pid = 0;
rw->rw_child_slot = 0;
rw->rw_crashed_at = 0;
rw->rw_shmem_slot = slotno;
rw->rw_terminate = false;
+ rw->rw_notify = false;
/* Log it! */
ereport(LOG,
(errmsg("registering background worker \"%s\"",
rw->rw_worker.bgw_name)));
slist_push_head(&BackgroundWorkerList, &rw->rw_lnode);
}
}
@@ -796,20 +797,21 @@ RegisterBackgroundWorker(BackgroundWorker *worker)
errmsg("out of memory")));
return;
}
rw->rw_worker = *worker;
rw->rw_backend = NULL;
rw->rw_pid = 0;
rw->rw_child_slot = 0;
rw->rw_crashed_at = 0;
rw->rw_terminate = false;
+ rw->rw_notify = false;
slist_push_head(&BackgroundWorkerList, &rw->rw_lnode);
}
/*
* Register a new background worker from a regular backend.
*
* Returns true on success and false on failure. Failure typically indicates
* that no background worker slots are currently available.
*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1757b4d..77cd3e0 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2919,20 +2919,30 @@ CleanupBackgroundWorker(int pid,
if ((rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
}
/*
+ * It's possible that this background worker started some OTHER
+ * background worker and asked to be notified when that worker
+ * started or stopped. If so, cancel any notifications destined
+ * for the now-dead background worker. Backends are dealt with
+ * separately below.
+ */
+ if (!rw->rw_backend && rw->rw_notify)
+ BackgroundWorkerStopNotifications(rw->rw_pid);
+
+ /*
* We must release the postmaster child slot whether this worker is
* connected to shared memory or not, but we only treat it as a crash
* if it is in fact connected.
*/
if (!ReleasePostmasterChildSlot(rw->rw_child_slot) &&
(rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
{
HandleChildCrash(pid, exitstatus, namebuf);
return true;
}
@@ -2952,20 +2962,21 @@ CleanupBackgroundWorker(int pid,
* started or stopped. If so, cancel any notifications destined
* for the now-dead backend.
*/
if (rw->rw_backend->bgworker_notify)
BackgroundWorkerStopNotifications(rw->rw_pid);
free(rw->rw_backend);
rw->rw_backend = NULL;
}
rw->rw_pid = 0;
rw->rw_child_slot = 0;
+ rw->rw_notify = false;
ReportBackgroundWorkerPID(rw); /* report child death */
LogChildExit(LOG, namebuf, pid, exitstatus);
return true;
}
return false;
}
@@ -3102,20 +3113,21 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
{
dlist_delete(&rw->rw_backend->elem);
#ifdef EXEC_BACKEND
ShmemBackendArrayRemove(rw->rw_backend);
#endif
free(rw->rw_backend);
rw->rw_backend = NULL;
}
rw->rw_pid = 0;
rw->rw_child_slot = 0;
+ rw->rw_notify = false;
/* don't reset crashed_at */
/* don't report child stop, either */
/* Keep looping so we can signal remaining workers */
}
else
{
/*
* This worker is still alive. Unless we did so already, tell it
* to commit hara-kiri.
*
@@ -5653,31 +5665,43 @@ maybe_start_bgworker(void)
/*
* When a backend asks to be notified about worker state changes, we
* set a flag in its backend entry. The background worker machinery needs
* to know when such backends exit.
*/
bool
PostmasterMarkPIDForWorkerNotify(int pid)
{
dlist_iter iter;
+ slist_iter siter;
Backend *bp;
dlist_foreach(iter, &BackendList)
{
bp = dlist_container(Backend, elem, iter.cur);
if (bp->pid == pid)
{
bp->bgworker_notify = true;
return true;
}
}
+
+ slist_foreach(siter, &BackgroundWorkerList)
+ {
+ RegisteredBgWorker *rbgw;
+ rbgw = slist_container(RegisteredBgWorker, rw_lnode, siter.cur);
+ if (rbgw->rw_pid == pid)
+ {
+ rbgw->rw_notify = true;
+ return true;
+ }
+ }
return false;
}
#ifdef EXEC_BACKEND
/*
* The following need to be available to the save/restore_backend_variables
* functions. They are marked NON_EXEC_STATIC in their home modules.
*/
extern slock_t *ShmemLock;
diff --git a/src/include/postmaster/bgworker_internals.h b/src/include/postmaster/bgworker_internals.h
index b0ab4c2..5ecaeca 100644
--- a/src/include/postmaster/bgworker_internals.h
+++ b/src/include/postmaster/bgworker_internals.h
@@ -25,20 +25,21 @@
*/
typedef struct RegisteredBgWorker
{
BackgroundWorker rw_worker; /* its registry entry */
struct bkend *rw_backend; /* its BackendList entry, or NULL */
pid_t rw_pid; /* 0 if not running */
int rw_child_slot;
TimestampTz rw_crashed_at; /* if not 0, time it last crashed */
int rw_shmem_slot;
bool rw_terminate;
+ bool rw_notify; /* gets background worker start/stop notifications */
slist_node rw_lnode; /* list link */
} RegisteredBgWorker;
extern slist_head BackgroundWorkerList;
extern Size BackgroundWorkerShmemSize(void);
extern void BackgroundWorkerShmemInit(void);
extern void BackgroundWorkerStateChange(void);
extern void ForgetBackgroundWorker(slist_mutable_iter *cur);
extern void ReportBackgroundWorkerPID(RegisteredBgWorker *);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
