Re: Proper way to reload config files in backend SIGHUP handler

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