Re: SIGHUP not received by custom bgworkers if postmaster is notified

Hi all,

While playing with custom background workers, I noticed that postmaster
does not notify its registered bgworkers if it receives SIGHUP,
so you have to send a SIGHUP directly to the bgworker process to notify it.
Signal handling is correctly done for SIGQUIT and SIGTERM for shutdown only.
Attached is a patch fixing that, I simply added a call to
SignalUnconnectedWorkers in SIGHUP_handler:postmaster.c.

Regards,
--
Michael

On 21-03-2013 05:06, Michael Paquier wrote:
> While playing with custom background workers, I noticed that postmaster does
> not notify its registered bgworkers if it receives SIGHUP,
> so you have to send a SIGHUP directly to the bgworker process to notify it.
> Signal handling is correctly done for SIGQUIT and SIGTERM for shutdown only.
> Attached is a patch fixing that, I simply added a call to
> SignalUnconnectedWorkers in SIGHUP_handler:postmaster.c.
>
Per this discussion [1], it seems it is as is by design. AFAICS controlling
when change configuration parameters is a feature not a bug. Alvaro said that
will include SIGHUP handle in worker_spi (see [2] for how to process
configurantion file).

[1] http://www.postgresql.org/message-id/20121231140353.GC4363@alvh.no-ip.org
[2]
http://www.postgresql.org/message-id/1357210591.1964.22.camel@localhost.localdomain

--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Euler Taveira escribió:
> On 21-03-2013 05:06, Michael Paquier wrote:
> > While playing with custom background workers, I noticed that postmaster does
> > not notify its registered bgworkers if it receives SIGHUP,
> > so you have to send a SIGHUP directly to the bgworker process to notify it.
> > Signal handling is correctly done for SIGQUIT and SIGTERM for shutdown only.
> > Attached is a patch fixing that, I simply added a call to
> > SignalUnconnectedWorkers in SIGHUP_handler:postmaster.c.
> >
> Per this discussion [1], it seems it is as is by design. AFAICS controlling
> when change configuration parameters is a feature not a bug. Alvaro said that
> will include SIGHUP handle in worker_spi (see [2] for how to process
> configurantion file).

They are opposite ends of the problem. Worker code needs a SIGHUP
signal handler, whatever that is (most likely something that causes the
configuration to be reread), which is what Guillaume's patch is about;
but postmaster needs to *send* a SIGHUP to its bgworker children, which
is what Michael is on about. Currently postmaster signals children that
are connected to shmem, but it's not considering those that aren't
connected.

At least that's how I understand the issue at hand, without actually
looking deeper into it.

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

On Fri, Mar 22, 2013 at 12:15 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com>wrote:

> Euler Taveira escribió:
> > On 21-03-2013 05:06, Michael Paquier wrote:
> > > While playing with custom background workers, I noticed that
> postmaster does
> > > not notify its registered bgworkers if it receives SIGHUP,
> > > so you have to send a SIGHUP directly to the bgworker process to
> notify it.
> > > Signal handling is correctly done for SIGQUIT and SIGTERM for shutdown
> only.
> > > Attached is a patch fixing that, I simply added a call to
> > > SignalUnconnectedWorkers in SIGHUP_handler:postmaster.c.
> > >
> > Per this discussion [1], it seems it is as is by design. AFAICS
> controlling
> > when change configuration parameters is a feature not a bug. Alvaro said
> that
> > will include SIGHUP handle in worker_spi (see [2] for how to process
> > configurantion file).
>
> They are opposite ends of the problem. Worker code needs a SIGHUP
> signal handler, whatever that is (most likely something that causes the
> configuration to be reread), which is what Guillaume's patch is about;
> but postmaster needs to *send* a SIGHUP to its bgworker children, which
> is what Michael is on about. Currently postmaster signals children that
> are connected to shmem, but it's not considering those that aren't
> connected.
>
> At least that's how I understand the issue at hand, without actually
> looking deeper into it.
>
Yes, that's exactly the problem. And I believe that the postmaster should
also notify its registered bgworkers if it receives a SIGHUP as it does for
its other backends. Have a look at the 1-line patch I sent to see how I
fixed
that...
--
Michael

Hi all,

Please find attached a simple example of bgworker that logs a message each
time a SIGTERM or SIGHUP signal is received by it:
- "hello signal: processed SIGHUP" when SIGHUP is handled by my example
- "hello signal: processed SIGTERM" when SIGTERM is handled by my example

With the current master code, here is what I get:
$ for i in {1..5}; do pg_ctl reload -D ~/bin/pgsql/master/; sleep 1; done
server signaled
server signaled
server signaled
server signaled
server signaled
$ cat ~/bin/pgsql/master/pg_log/postgresql-2013-03-23_112246.log
LOG: starting background worker process "hello signal worker"
LOG: database system was shut down at 2013-03-23 11:22:46 JST
LOG: database system is ready to accept connections
LOG: autovacuum launcher started

LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files
LOG: received SIGHUP, reloading configuration files

SIGHUP is not received by my bgworker. But SIGTERM is:

$ tail -n 5 ~/bin/pgsql/master/pg_log/postgresql-2013-03-23_112246.log
LOG: autovacuum launcher shutting down
*LOG: hello signal: processed SIGTERM*
LOG: worker process: hello signal worker (PID 2873) exited with exit code 0
LOG: shutting down
LOG: database system is shut down

Now, if I apply my fix and redo the same tests, here is what I get:

$ for i in {1..5}; do pg_ctl reload -D ~/bin/pgsql/master/; sleep 1; done
server signaled
server signaled
server signaled
server signaled
server signaled
$ cat ~/bin/pgsql/master/pg_log/postgresql-2013-03-23_113315.log
LOG: starting background worker process "hello signal worker"
LOG: database system was shut down at 2013-03-23 11:33:14 JST
LOG: database system is ready to accept connections
LOG: autovacuum launcher started

LOG: received SIGHUP, reloading configuration files
*LOG: hello signal: processed SIGHUP*
LOG: received SIGHUP, reloading configuration files
*LOG: hello signal: processed SIGHUP*
LOG: received SIGHUP, reloading configuration files
*LOG: hello signal: processed SIGHUP*
LOG: received SIGHUP, reloading configuration files
*LOG: hello signal: processed SIGHUP*
LOG: received SIGHUP, reloading configuration files
*LOG: hello signal: processed SIGHUP*

So SIGHUP is now correctly managed by the bgworker. As well as SIGTERM:

$ pg_ctl st: pg_ctl stop -D ~/bin/pgsql/master/
waiting for server to shut down.... done
server stopped
ioltas(at)nukkle:~/bin/extra(linux OK)$ tail -n 5
~/bin/pgsql/master/pg_log/postgresql-2013-03-23_113315.log
*LOG: hello signal: processed SIGTERM*
LOG: autovacuum launcher shutting down
LOG: worker process: hello signal worker (PID 13781) exited with exit code
0
LOG: shutting down
LOG: database system is shut down

It would be great to get that fixed.
Thanks.
--
Michael

Michael Paquier escribió:
> Hi all,
>
> While playing with custom background workers, I noticed that postmaster
> does not notify its registered bgworkers if it receives SIGHUP,
> so you have to send a SIGHUP directly to the bgworker process to notify it.
> Signal handling is correctly done for SIGQUIT and SIGTERM for shutdown only.
> Attached is a patch fixing that, I simply added a call to
> SignalUnconnectedWorkers in SIGHUP_handler:postmaster.c.

Thanks, applied.

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

Michael Paquier escribió:
> Hi all,
>
> Please find attached a simple example of bgworker that logs a message each
> time a SIGTERM or SIGHUP signal is received by it:
> - "hello signal: processed SIGHUP" when SIGHUP is handled by my example
> - "hello signal: processed SIGTERM" when SIGTERM is handled by my example

I committed some improvements to worker_spi this morning that I think
enough demostrate signal handling capabilities, which I think is what
your submitted code would do. If you see more use for a separate body
of sample worker code, by all means do submit that.

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

Thanks for committing the fix!

On Thu, Apr 11, 2013 at 4:11 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>wrote:

> Michael Paquier escribió:
> > Hi all,
> >
> > Please find attached a simple example of bgworker that logs a message
> each
> > time a SIGTERM or SIGHUP signal is received by it:
> > - "hello signal: processed SIGHUP" when SIGHUP is handled by my example
> > - "hello signal: processed SIGTERM" when SIGTERM is handled by my example
>
> I committed some improvements to worker_spi this morning that I think
> enough demostrate signal handling capabilities, which I think is what
> your submitted code would do. If you see more use for a separate body
> of sample worker code, by all means do submit that.
>
Sure.
--
Michael