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,
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]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7389aad.
[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.)
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