Re: dynamic background workers, round two

On Thu, Jul 25, 2013 at 6:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> Hi,
>
> On 2013-07-24 12:46:21 -0400, Robert Haas wrote:
> > The attached patch attempts to remedy this problem. When you register
> > a background worker, you can obtain a "handle" that can subsequently
> > be used to query for the worker's PID. If you additionally initialize
> > bgw_notify_pid = MyProcPid, then the postmaster will send you SIGUSR1
> > when worker startup has been attempted (successfully or not). You can
> > wait for this signal by passing your handle to
> > WaitForBackgroundWorkerStartup(), which will return only when either
> > (1) an attempt has been made to start the worker or (2) the postmaster
> > is determined to have died. This interface seems adequate for
> > something like worker_spi, where it's useful to know whether the child
> > was started or not (and return the PID if so) but that's about all
> > that's really needed.
>
> This seems like a sensible idea to me. But, in the context of dynamic
> query, don't we also need the reverse infrastructure of notifying a
> bgworker that the client, that requested it to be started, has died?
> Ending up with a dozen bgworkers running because the normal backend
> FATALed doesn't seem nice.
>
Yes, this would be something necessary to have, but definitely it should be
a separate patch. In order to satisfy that, you could register for each
worker the PID of the parent process that started it, if it was started by
an an other bgworker, and have postmaster scan the list of bgworkers that
need to be stopped after the death of their parent if it was a bgworker. By
the way, the PID registered in this case should not be bgw_notify_pid but
something saved in BackgroundWorkerSlot. My 2c on that though, feel free to
comment.

> More complex notification interfaces can also be coded using the
> > primitives introduced by this patch. Setting bgw_notify_pid will
> > cause the postmaster to send SIGUSR1 every time it starts the worker
> > and every time the worker dies. Every time you receive that signal,
> > you can call GetBackgroundWorkerPid() for each background worker to
> > find out which ones have started or terminated. The only slight
> > difficulty is in waiting for receipt of that signal, which I
> > implemented by adding a new global called set_latch_on_sigusr1.
> > WaitForBackgroundWorkerStartup() uses that flag internally, but
> > there's nothing to prevent a caller from using it as part of their own
> > event loop. I find the set_latch_on_sigusr1 flag to be slightly
> > ugly, but it seems better and far safer than having the postmaster try
> > to actually set the latch itself - for example, it'd obviously be
> > unacceptable to pass a Latch * to the postmaster and have the
> > postmaster assume that pointer valid.
>
> I am with you with finding it somewhat ugly.
>
After a quick look at the code, yeah using a boolean here looks like an
overkill.
--
Michael