Proper way to reload config files in backend SIGHUP handler

2018-05-03 19:25 GMT-03:00 Mike Palmiotto <mike.palmiotto@crunchydata.com>:

I am writing a PostgreSQL extension and need to reload certain
extension-managed files whenever the postmaster is reloaded/receives SIGHUP,
but cannot find anything that looks like a standard way to do that. Is there a
hook or recommended method or something else I am missing?

Signals are initially blocked in bgworker. You have to unblock them
(see BackgroundWorkerUnblockSignals) to allow your extension to
customize its signal handlers. See an example in worker_spi test
module.

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

On 05/03/2018 08:43 PM, Euler Taveira wrote:

2018-05-03 19:25 GMT-03:00 Mike Palmiotto <mike.palmiotto@crunchydata.com>:

I am writing a PostgreSQL extension and need to reload certain
extension-managed files whenever the postmaster is reloaded/receives SIGHUP,
but cannot find anything that looks like a standard way to do that. Is there a
hook or recommended method or something else I am missing?

Signals are initially blocked in bgworker. You have to unblock them
(see BackgroundWorkerUnblockSignals) to allow your extension to
customize its signal handlers. See an example in worker_spi test
module.

I don't seem to have any problem catching the SIGHUP in my extension without
BackgroundWorkerUnblockSignals a la:

----

pqsignal_sighup(SIGHUP, sighup_handler);
.
.
.
static void
sighup_handler(SIGNAL_ARGS)
{
<parse configs>
}

----

Perhaps the signal is already unblocked at this point. Unblocking signals
first doesn't appear to have any affect. Am I missing something here?

I noticed that 6e1dd27
(https://github.com/postgres/postgres/commit/6e1dd2773eb60a6ab87b27b8d9391b756e904ac3)
introduced a ConfigReloadPending flag that can be set by calling
PostgresSigHupHandler inside the extension's handler, which seemingly allows
things to work as they usually would, and then we can go on to do other config
processing.

Is this approach kosher, or is there another preferred route?

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

On Fri, May 04, 2018 at 12:47:09AM -0400, Mike Palmiotto wrote:

I don't seem to have any problem catching the SIGHUP in my extension
without > BackgroundWorkerUnblockSignals a la:

pqsignal_sighup(SIGHUP, sighup_handler);

I am pretty sure you mean s/pqsignal_sighup/pqsignal/.

static void
sighup_handler(SIGNAL_ARGS)
{
<parse configs>
}

Signal handlers should not do anything complicated, and should access
only variables of the type volatile sig_atomic_t. This is also in the
documentation here (see Writing Signal Handlers):
https://www.postgresql.org/docs/devel/static/source-conventions.html

Perhaps the signal is already unblocked at this point. Unblocking signals
first doesn't appear to have any affect. Am I missing something here?

I noticed that 6e1dd27
(https://github.com/postgres/postgres/commit/6e1dd2773eb60a6ab87b27b8d9391b756e904ac3)
introduced a ConfigReloadPending flag that can be set by calling
PostgresSigHupHandler inside the extension's handler, which seemingly allows
things to work as they usually would, and then we can go on to do other config
processing.

Is this approach kosher, or is there another preferred route?

From the documentation of
https://www.postgresql.org/docs/devel/static/bgworker.html:

Signals are initially blocked when control reaches the background
worker's main function, and must be unblocked by it; this is to allow
the process to customize its signal handlers, if necessary. Signals can
be unblocked in the new process by calling
BackgroundWorkerUnblockSignals and blocked by calling
BackgroundWorkerBlockSignals.

I have for ages in one of my github repositories a small example of
bgworker which covers signal handling, located here:
https://github.com/michaelpq/pg_plugins/tree/master/hello_signal

If you quote the call to BackgroundWorkerUnblockSignals, you would see
that the SIGHUP is not received by the process, which is what the
documentation says, and what I would expect as well.
--
Michael

On 05/04/2018 09:35 AM, Michael Paquier wrote:

On Fri, May 04, 2018 at 12:47:09AM -0400, Mike Palmiotto wrote:

I don't seem to have any problem catching the SIGHUP in my extension
without > BackgroundWorkerUnblockSignals a la:

pqsignal_sighup(SIGHUP, sighup_handler);

I am pretty sure you mean s/pqsignal_sighup/pqsignal

Yeah, sorry, that's what I meant. :)

static void
sighup_handler(SIGNAL_ARGS)
{
<parse configs>
}

Signal handlers should not do anything complicated, and should access
only variables of the type volatile sig_atomic_t. This is also in the
documentation here (see Writing Signal Handlers):
https://www.postgresql.org/docs/devel/static/source-conventions.html

Perhaps the signal is already unblocked at this point. Unblocking signals
first doesn't appear to have any affect. Am I missing something here?

I noticed that 6e1dd27
(https://github.com/postgres/postgres/commit/6e1dd2773eb60a6ab87b27b8d9391b756e904ac3)
introduced a ConfigReloadPending flag that can be set by calling
PostgresSigHupHandler inside the extension's handler, which seemingly allows
things to work as they usually would, and then we can go on to do other config
processing.

Is this approach kosher, or is there another preferred route?

From the documentation of
https://www.postgresql.org/docs/devel/static/bgworker.html:

Signals are initially blocked when control reaches the background
worker's main function, and must be unblocked by it; this is to allow
the process to customize its signal handlers, if necessary. Signals can
be unblocked in the new process by calling
BackgroundWorkerUnblockSignals and blocked by calling
BackgroundWorkerBlockSignals.

I have for ages in one of my github repositories a small example of
bgworker which covers signal handling, located here:
https://github.com/michaelpq/pg_plugins/tree/master/hello_signal

If you quote the call to BackgroundWorkerUnblockSignals, you would see
that the SIGHUP is not received by the process, which is what the
documentation says, and what I would expect as well.

Ah, that explains it. I'm catching SIGHUP in my extension (backend) without
using a background worker. Would writing a background worker and registering
it in PG_init allow us to reload the extension-specific config changes in the
backend? It seems like that would only affect the memory of the worker process
itself and not propagate to the backend. Perhaps that's an inaccurate assumption.

The end goal is:
1) Update extension-specific configs
2) Issue a postgres reload (SIGHUP)
3) Update extension variables in connected backend according to configs

That all _appears_ to work with the following:
1) Call PostgresSigHupHandler (set ConfigReloadPending, SetLatch(MyLatch) and
let the internal code handle that normally)
2) Read in custom extension-specific config files and modify non-volatile,
non-sig_atomic_t variables that are specific to the extension

It's possible that issues could arise with different levels of optimization. I
haven't tested that out.

As you've indicated, it's unsafe to do anything other than SetLatch/modify
volatile sig_atomic_t variables. If this means modifying extension-specific
non-volatile, non-sig_atomic_t variables is also hazardous, we should probably
just stick to reloading these configs using a custom SQL function, rather than
using a sighup handler.

Thanks for your patience in this line of questioning.

Regards,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com