Thread: Usage of ProcessConfigfile in SIGHUP_Handler
Hi pgsql hacker,
Recently I have been trying to understand why GUC changes will be visible even though they are done in the signal handler as part of ProcessConfigfile (done in some extension code). Later I have seen almost all postgresql processes/bgworkers use signal handler to set a variable ConfigReloadPending which will later be read in main code to process guc changes but for postmaster ProcessConfigfile is being called from signal handler itself which intern has memory allocation related code (non-async safe code). Is it safe to do this?
Regards,
Recently I have been trying to understand why GUC changes will be visible even though they are done in the signal handler as part of ProcessConfigfile (done in some extension code). Later I have seen almost all postgresql processes/bgworkers use signal handler to set a variable ConfigReloadPending which will later be read in main code to process guc changes but for postmaster ProcessConfigfile is being called from signal handler itself which intern has memory allocation related code (non-async safe code). Is it safe to do this?
Regards,
Narayana
On Thu, Aug 22, 2024 at 05:37:13PM +0530, Lakshmi Narayana Velayudam wrote: > Later I have seen almost > all postgresql processes/bgworkers use signal handler to set a > variable *ConfigReloadPending > *which will later be read in main code to process guc changes but for > postmaster *ProcessConfigfile *is being called from signal handler itself > which intern has memory allocation related code (non-async safe code). Is > it safe to do this? I think this is no longer true as of v16, thanks to commit 7389aad [0]. [0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7389aad -- nathan
On Thu, Aug 22, 2024 at 8:46 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I think this is no longer true as of v16, thanks to commit 7389aad [0].
My Bad Nathan, was looking at PG 11, 14 codes. Just to be sure, calling ProcessConfigFile is a bug from a signal handler is a bug, right? Since it uses AllocSetContextCreate & also GUC variables changes might not be visible in regular flow.
I saw the discussion of the commit but couldn't conclude from the discussion that it was changed due to ProcessConfigFIle.
Regards,
Narayana
Lakshmi Narayana Velayudam <dev.narayana.v@gmail.com> writes:
> My Bad Nathan, was looking at PG 11, 14 codes. Just to be sure, calling
> *ProcessConfigFile *is a bug from a signal handler is a bug, right?
No, it was not. The previous postmaster coding blocked signals
everywhere except immediately around the main loop's select() call,
so there wasn't any real hazard of signal handlers interrupting
anything of concern. We redid it for cleanliness, not because there
was any observable bug.
(If there had been a bug there, ProcessConfigFile would have been
the least of our problems, because all the other postmaster signals
were handled in the same style.)
regards, tom lane
On Thu, Aug 22, 2024 at 9:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The previous postmaster coding blocked signals
> everywhere except immediately around the main loop's select() call,
> so there wasn't any real hazard of signal handlers interrupting
> anything of concern. We redid it for cleanliness, not because there
> was any observable bug.
Agreed, signal handlers are very sensitive (at least as of this moment) and the current approach looks very clean and safe.
> No, it was not.
> (If there had been a bug there, ProcessConfigFile would have been
> the least of our problems, because all the other postmaster signals
> were handled in the same style.)
> the least of our problems, because all the other postmaster signals
> were handled in the same style.)
Just as an info for future readers, it is indeed a bug for two reasons
1. changin GUC values in the signal handler, compiler might optimise the values so main control wouldn't have know about it later (90% sure about this)
2. ProcessConfigFile calls AllocSetCreate which in turn calls malloc which is not async signal safe(see man 7 signal-safety) and can cause deadlock in certain situations. (Dig malloc internal code if interested)
Not only in SIGHUP handler it is done in other handlers as well. Feel free to correct me if there is any inaccuracy in what I said.
Regards,
Narayana
Lakshmi Narayana Velayudam <dev.narayana.v@gmail.com> writes:
> Just as an info for future readers, it is indeed a bug for two reasons
No, it isn't. There's twenty years' worth of successful usage of
the old coding pattern that says you're wrong.
regards, tom lane