Re: [v9.3] Extra Daemons (Re: elegant and effective way for running jobs inside a database) - Mailing list pgsql-hackers

Kohei KaiGai escribió:
> 2012/10/22 Alvaro Herrera <alvherre@2ndquadrant.com>:
> > Here's an updated version of this patch, which also works in
> > an EXEC_BACKEND environment.  (I haven't tested this at all on Windows,
> > but I don't see anything that would create a portability problem there.)
> >
> I also tried to check the latest patch "briefly".
> Let me comment on several points randomly.
>
> Once bgworker process got crashed, postmaster tries to restart it about 60
> seconds later. My preference is this interval being configurable with initial
> registration parameter; that includes "never restart again" option.
> Probably, some extensions don't want to restart again on unexpected crash.

I changed this.

> Stop is one other significant event for bgworker, not only start-up.
> This interface has no way to specify when we want to stop bgworker.
> Probably, we can offer two options. Currently, bgworkers are simultaneously
> terminated with other regular backends. In addition, I want an option to make
> sure bgworker being terminated after all the regular backends exit.
> It is critical issue for me, because I try to implement parallel
> calculation server
> with bgworker, thus, it should not be terminated earlier than regular backend.

I am not really sure about this.  Each new behavior we want to propose
requires careful attention, because we need to change postmaster's
shutdown sequence in pmdie(), and also reaper() and
PostmasterStateMachine().  After looking into it, I am hesitant to
change this too much unless we can reach a very detailed agreement of
exactly what we want to happen.

Also note that there are two types of workers: those that require a
database connection, and those that do not.  The former are signalled
via SignalSomeChildren(BACKEND_TYPE_BGWORKER); the latter are signalled
via SignalUnconnectedWorker().  Would this distinction be enough for
you?

Delivering more precise shutdown signals is tricky; we would have to
abandon SignalSomeChildren() and code something new to scan the workers
list checking the stop time for each one.  (Except in emergency
situations, of course, in which we would continue to rely on
SignalSomeChildren).

> How about to move bgw_name field into tail of BackgroundWorker structure?
> It makes simplifies the logic in RegisterBackgroundWorker(), if it is
> located as:
>     typedef struct BackgroundWorker
>     {
>         int         bgw_flags;
>         BgWorkerStartTime bgw_start_time;
>         bgworker_main_type  bgw_main;
>         void       *bgw_main_arg;
>         bgworker_sighdlr_type bgw_sighup;
>         bgworker_sighdlr_type bgw_sigterm;
>         char        bgw_name[1];  <== (*)
>     } BackgroundWorker;

This doesn't work; or rather, it works and it makes
RegisterBackgroundWorker() code somewhat nicer, but according to Larry
Wall's Conservation of Cruft Principle, the end result is that the
module code to register a new worker is a lot messier; it can no longer
use a struct in the stack, but requires malloc() or similar.  I don't
see that this is a win overall.

> StartOneBackgroundWorker always scan the BackgroundWorkerList from
> the head. Isn't it available to save the current position at static variable?
> If someone tries to manage thousand of bgworkers, it makes a busy loop. :(

Seems messy; we would have to get into the guts of slist_foreach (unroll
the macro and make the iterator static).  I prefer not to go that path,
at least not for now.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services