Re: [Patch] Create a new session in postmaster by calling setsid()
On 30/12/2018 22:56, Tom Lane wrote: This comment needs copy-editing: + * If the user hits interrupts the startup (e.g. with CTRL-C), we'd Looks good otherwise. Thanks, fixed that, and pushed. - Heikki
Re: [Patch] Create a new session in postmaster by calling setsid()
Heikki Linnakangas writes: > Here's a patch to implement that. Seems to work. There is a small window > between launching postmaster and installing the signal handler, though, > where CTRL-C on pg_ctl will not abort the server launch. I think that's > acceptable. Hm ... you could partially forestall that by installing the signal handler first. But then you have to assume that storing a pid_t variable is atomic, which perhaps is bad? Though it's hard to credit that any platform would need multiple instructions to do that. Also, I suppose there's still a window, since the fork will necessarily occur some time before you're able to store the child PID into the variable. Another possible idea is to block SIGINT from before the fork till after you've set the variable. But that seems overly complicated, and perhaps not without problems of its own. So on the whole I concur with your approach. > Forgot attachment, here it is. This comment needs copy-editing: + * If the user hits interrupts the startup (e.g. with CTRL-C), we'd Looks good otherwise. regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
On 30/12/2018 21:59, Heikki Linnakangas wrote:
On 28/12/2018 22:13, Tom Lane wrote:
Heikki Linnakangas writes:
Another idea would be to call setsid() in pg_ctl, after forking
postmaster, like in Paul's original patch. That solves 1. and 2. To
still do 3., add a signal handler for SIGINT in pg_ctl, which forwards
the SIGINT to the postmaster process. Thoughts on that?
That seems like a nice idea.
Here's a patch to implement that.
Forgot attachment, here it is.
- Heikki
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 1d0b056dde0..cb7b88bc7df 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -103,12 +103,13 @@ static char backup_file[MAXPGPATH];
static char promote_file[MAXPGPATH];
static char logrotate_file[MAXPGPATH];
+static volatile pgpid_t postmasterPID = -1;
+
#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
static SERVICE_STATUS status;
static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
static HANDLE shutdownHandles[2];
-static pid_t postmasterPID = -1;
#define shutdownEvent shutdownHandles[0]
#define postmasterProcess shutdownHandles[1]
@@ -471,6 +472,20 @@ start_postmaster(void)
/* fork succeeded, in child */
+ /*
+ * If possible, detach the postmaster process from the launching process group
+ * and make it a group leader, so that it doesn't get signaled along with the
+ * current group that launched it.
+ */
+#ifdef HAVE_SETSID
+ if (setsid() < 0)
+ {
+ write_stderr(_("%s: could not start server due to setsid() failure: %s\n"),
+ progname, strerror(errno));
+ exit(1);
+ }
+#endif
+
/*
* Since there might be quotes to handle here, it is easier simply to pass
* everything to a shell to process them. Use exec so that the postmaster
@@ -719,6 +734,30 @@ read_post_opts(void)
}
}
+/*
+ * SIGINT signal handler used while waiting for postmaster to start up.
+ * Forwards the SIGINT to the postmaster process, asking it to shut down,
+ * before terminating pg_ctl itself. This way, if the user hits CTRL-C while
+ * waiting for the server to start up, the server launch is aborted.
+ */
+static void
+trap_sigint_during_startup(int sig)
+{
+ if (postmasterPID != -1)
+ {
+ if (kill(postmasterPID, SIGINT) != 0)
+ write_stderr(_("%s: could not send stop signal (PID: %ld): %s\n"),
+ progname, (pgpid_t) postmasterPID, strerror(errno));
+ }
+
+ /*
+ * Clear the signal handler, and send the signal again, to terminate the
+ * process as normal.
+ */
+ pqsignal(SIGINT, SIG_DFL);
+ raise(SIGINT);
+}
+
static char *
find_other_exec_or_die(const char *argv0, const char *target, const char *versionstr)
{
@@ -827,6 +866,17 @@ do_start(void)
if (do_wait)
{
+ /*
+ * If the user hits interrupts the startup (e.g. with CTRL-C), we'd
+ * like to abort the server launch. Install a signal handler that
+ * will forward SIGINT to the postmaster process, while we wait.
+ *
+ * (We don't bother to reset the signal handler after the launch,
+ * as we're about to exit, anyway.)
+ */
+ postmasterPID = pm_pid;
+ pqsignal(SIGINT, trap_sigint_during_startup);
+
print_msg(_("waiting for server to start..."));
switch (wait_for_postmaster(pm_pid, false))
Re: [Patch] Create a new session in postmaster by calling setsid()
Heikki Linnakangas writes: > We talked about adding a flag to postmaster, to tell it to call > setsid(). But I'm not sure what would be the appropriate time to do it. Yeah, there's no ideal time in the postmaster. > Another idea would be to call setsid() in pg_ctl, after forking > postmaster, like in Paul's original patch. That solves 1. and 2. To > still do 3., add a signal handler for SIGINT in pg_ctl, which forwards > the SIGINT to the postmaster process. Thoughts on that? That seems like a nice idea. regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
On 30/09/2018 15:47, Michael Paquier wrote: On Thu, Sep 13, 2018 at 12:24:38PM -0700, Andres Freund wrote: On 2018-09-13 15:27:58 +0800, Paul Guo wrote: From the respective of users instead of developers, I think for such important service, tty should be dissociated, i.e. postmaster should be daemonized by default (and even should have default log filename?) or be setsid()-ed at least. I don't think it'd be good to switch to postgres daemonizing itself. If we were starting fresh, maybe, but there's plenty people invoking postgres from scripts etc. And tools like systemd don't actually want daemons to fork away, so there's no clear need from that side either. It seems to me that the thread is pointing out that we would not want the postmaster to daemonize itself, but that something in pg_ctl could be introduced, potentially optional. I am marking this patch as returned with feedback. Hmm. So, the requirements are: 1. When launched from pg_ctl, postmaster should become leader of a new session and process group. To address Paul's original scenario. 2. If "postmaster" is launched stand-alone from terminal or a script, it should stay in foreground, in the parent session, so that it can be killed with Ctrl-C. 3. Hitting Ctrl-C while "pg_ctl start -w" is waiting for postmaster to start up, should "abort" and kill postmaster. We talked about adding a flag to postmaster, to tell it to call setsid(). But I'm not sure what would be the appropriate time to do it. If it's done early during postmaster startup, then we still wouldn't have solved 3. Hitting Ctrl-C on pg_ctl, while the server is still in recovery, would not kill the server. We could do it after recovery has finished, but if we have already launched auxiliar processes, I think they would stay in the old process group and session. So I don't think that works. Another idea would be to call setsid() in pg_ctl, after forking postmaster, like in Paul's original patch. That solves 1. and 2. To still do 3., add a signal handler for SIGINT in pg_ctl, which forwards the SIGINT to the postmaster process. Thoughts on that? - Heikki
Re: [Patch] Create a new session in postmaster by calling setsid()
On Thu, Sep 13, 2018 at 12:24:38PM -0700, Andres Freund wrote: > On 2018-09-13 15:27:58 +0800, Paul Guo wrote: >> From the respective of users instead of developers, I think for such >> important >> service, tty should be dissociated, i.e. postmaster should be daemonized by >> default (and even should have default log filename?) or be setsid()-ed at >> least. > > I don't think it'd be good to switch to postgres daemonizing itself. If > we were starting fresh, maybe, but there's plenty people invoking > postgres from scripts etc. And tools like systemd don't actually want > daemons to fork away, so there's no clear need from that side either. It seems to me that the thread is pointing out that we would not want the postmaster to daemonize itself, but that something in pg_ctl could be introduced, potentially optional. I am marking this patch as returned with feedback. -- Michael signature.asc Description: PGP signature
Re: [Patch] Create a new session in postmaster by calling setsid()
Hi, On 2018-09-13 15:27:58 +0800, Paul Guo wrote: > From the respective of users instead of developers, I think for such > important > service, tty should be dissociated, i.e. postmaster should be daemonized by > default (and even should have default log filename?) or be setsid()-ed at > least. I don't think it'd be good to switch to postgres daemonizing itself. If we were starting fresh, maybe, but there's plenty people invoking postgres from scripts etc. And tools like systemd don't actually want daemons to fork away, so there's no clear need from that side either. > I'm not sure the several-years-ago LINUX_OOM_ADJ issue (to resolve that, > silient_mode was removed) still exists or not. I don't have context about > that, so > I conceded to use setsid() even I prefer the deleted silent_mode code. Yes, the OOM issues are still relevant. Greetings, Andres Freund
Re: [Patch] Create a new session in postmaster by calling setsid()
On Thu, Sep 13, 2018 at 8:20 AM, Michael Paquier wrote: > On Wed, Sep 12, 2018 at 03:55:00PM -0400, Tom Lane wrote: > > Although pg_ctl could sneak it in between forking and execing, it seems > > like it'd be cleaner to have the postmaster proper do it in response to > > a switch that pg_ctl passes it. That avoids depending on the fork/exec > > separation, and makes the functionality available for other postmaster > > startup mechanisms, and opens the possibility of delaying the detach > > to the end of startup. > > I would be fine with something happening only once the postmaster thinks > that consistency has been reached and is open for business, though I'd > still think that this should be controlled by a switch, where the > default does what we do now. Feel free to ignore me if I am outvoted ;) > -- > Michael > >From the respective of users instead of developers, I think for such important service, tty should be dissociated, i.e. postmaster should be daemonized by default (and even should have default log filename?) or be setsid()-ed at least. For concerns from developers, maybe a shell alias, or an internal switch in pg_ctl, or env variable guard in postmaster code could address. I'm not sure the several-years-ago LINUX_OOM_ADJ issue (to resolve that, silient_mode was removed) still exists or not. I don't have context about that, so I conceded to use setsid() even I prefer the deleted silent_mode code.
Re: [Patch] Create a new session in postmaster by calling setsid()
On Wed, Sep 12, 2018 at 03:55:00PM -0400, Tom Lane wrote: > Although pg_ctl could sneak it in between forking and execing, it seems > like it'd be cleaner to have the postmaster proper do it in response to > a switch that pg_ctl passes it. That avoids depending on the fork/exec > separation, and makes the functionality available for other postmaster > startup mechanisms, and opens the possibility of delaying the detach > to the end of startup. I would be fine with something happening only once the postmaster thinks that consistency has been reached and is open for business, though I'd still think that this should be controlled by a switch, where the default does what we do now. Feel free to ignore me if I am outvoted ;) -- Michael signature.asc Description: PGP signature
Re: [Patch] Create a new session in postmaster by calling setsid()
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> Hmph. Can't we just ignore that error? > If you ignore the error from setsid(), then you're still a process group > leader (as you would be after running setsid()), but you're still > attached to whatever controlling terminal (if any) you were previously > attached to. Oh, got it. So actually, the setsid has to be done by what is/will be the postmaster process. Although pg_ctl could sneak it in between forking and execing, it seems like it'd be cleaner to have the postmaster proper do it in response to a switch that pg_ctl passes it. That avoids depending on the fork/exec separation, and makes the functionality available for other postmaster startup mechanisms, and opens the possibility of delaying the detach to the end of startup. regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
> "Tom" == Tom Lane writes: >> The tricky part about doing setsid() is this: you're not allowed to >> do it if you're already a process group leader. silent_mode worked >> by having postmaster do another fork, exit in the parent, and do >> setsid() in the child. Tom> Hmph. Can't we just ignore that error? If you ignore the error from setsid(), then you're still a process group leader (as you would be after running setsid()), but you're still attached to whatever controlling terminal (if any) you were previously attached to. -- Andrew (irc:RhodiumToad)
Re: [Patch] Create a new session in postmaster by calling setsid()
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> We'd likely need a switch to control that. If memory serves, there > Tom> used to be such a switch, but we got rid of the postmaster's > Tom> setsid call and the switch too. We probably should dig in the > Tom> archives and review the reasoning about that. > The tricky part about doing setsid() is this: you're not allowed to do > it if you're already a process group leader. silent_mode worked by > having postmaster do another fork, exit in the parent, and do setsid() > in the child. Hmph. Can't we just ignore that error? regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
> "Tom" == Tom Lane writes: Tom> BTW, just thinking outside the box a bit --- perhaps the ideal Tom> behavior to address Michael's use-case would be to have the Tom> postmaster itself do setsid(), but not until it reaches the state Tom> of being ready to accept client connections. Tom> We'd likely need a switch to control that. If memory serves, there Tom> used to be such a switch, but we got rid of the postmaster's Tom> setsid call and the switch too. We probably should dig in the Tom> archives and review the reasoning about that. The old silent_mode switch? The tricky part about doing setsid() is this: you're not allowed to do it if you're already a process group leader. silent_mode worked by having postmaster do another fork, exit in the parent, and do setsid() in the child. If postmaster is launched from pg_ctl, then it won't be a group leader unless pg_ctl made it one. But when it's run from the shell, it will be if the shell does job control (the initial command of each shell job is the group leader); if it's run from a service management process, then it'll depend on what that process does. -- Andrew (irc:RhodiumToad)
Re: [Patch] Create a new session in postmaster by calling setsid()
I wrote: > Michael Paquier writes: >> Hmm. This patch breaks a feature of pg_ctl that I am really fond of for >> development. When starting a node which enters in recovery, I sometimes >> use Ctrl-C to stop pg_ctl, which automatically makes the started >> Postgres instance to stop, and this saves more strokes. With your >> patch, you don't get that anymore: when issuing Ctrl-C on pg_ctl then >> the started instance still runs in the background. I would be ready to >> accept a patch which does not change the default behavior, and makes the >> deamonization behavior activated only if an option switch is given by >> the user, like -d/--daemon. So I am -1 for what is proposed in its >> current shape. > Hmm, that seems like a pretty niche usage. I don't object to having > a switch to control this, but it seems to me that dissociating from > the terminal is by far the more commonly wanted behavior and so > ought to be the default. BTW, just thinking outside the box a bit --- perhaps the ideal behavior to address Michael's use-case would be to have the postmaster itself do setsid(), but not until it reaches the state of being ready to accept client connections. We'd likely need a switch to control that. If memory serves, there used to be such a switch, but we got rid of the postmaster's setsid call and the switch too. We probably should dig in the archives and review the reasoning about that. I'm still of the opinion that dissociating from the terminal ought to be the default. On at least some platforms, that happens automatically because the postmaster's stdin, stdout, and stderr have been redirected away from the terminal. If we don't do it on platforms where setsid() is necessary, then we have a cross-platform behavioral difference, which generally doesn't seem like a good thing. regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
Michael Paquier writes: > On Mon, Aug 06, 2018 at 12:11:26PM +0800, Paul Guo wrote: >> Yes, if considering the case of starting postmaster manually, we can not >> create >> a new session in postmaster, so pg_ctl seems to be a good place for setsid() >> call. Attached a newer patch. Thanks. > Hmm. This patch breaks a feature of pg_ctl that I am really fond of for > development. When starting a node which enters in recovery, I sometimes > use Ctrl-C to stop pg_ctl, which automatically makes the started > Postgres instance to stop, and this saves more strokes. With your > patch, you don't get that anymore: when issuing Ctrl-C on pg_ctl then > the started instance still runs in the background. I would be ready to > accept a patch which does not change the default behavior, and makes the > deamonization behavior activated only if an option switch is given by > the user, like -d/--daemon. So I am -1 for what is proposed in its > current shape. Hmm, that seems like a pretty niche usage. I don't object to having a switch to control this, but it seems to me that dissociating from the terminal is by far the more commonly wanted behavior and so ought to be the default. regards, tom lane
Re: [Patch] Create a new session in postmaster by calling setsid()
On Mon, Aug 06, 2018 at 12:11:26PM +0800, Paul Guo wrote: > Yes, if considering the case of starting postmaster manually, we can not > create > a new session in postmaster, so pg_ctl seems to be a good place for setsid() > call. Attached a newer patch. Thanks. Hmm. This patch breaks a feature of pg_ctl that I am really fond of for development. When starting a node which enters in recovery, I sometimes use Ctrl-C to stop pg_ctl, which automatically makes the started Postgres instance to stop, and this saves more strokes. With your patch, you don't get that anymore: when issuing Ctrl-C on pg_ctl then the started instance still runs in the background. I would be ready to accept a patch which does not change the default behavior, and makes the deamonization behavior activated only if an option switch is given by the user, like -d/--daemon. So I am -1 for what is proposed in its current shape. -- Michael signature.asc Description: PGP signature
Re: [Patch] Create a new session in postmaster by calling setsid()
On Thu, Aug 2, 2018 at 10:30 PM, Tom Lane wrote: > Paul Guo writes: > > [ make the postmaster execute setsid() too ] > > I'm a bit skeptical of this proposal. Forcing the postmaster to > dissociate from its controlling terminal is a good thing in some > scenarios, but probably less good in others, and manual postmaster > starts are probably mostly in the latter class. > > I wonder whether having "pg_ctl start" do a setsid() would accomplish > the same result with less chance of breaking debugging usages. > > regards, tom lane > Yes, if considering the case of starting postmaster manually, we can not create a new session in postmaster, so pg_ctl seems to be a good place for setsid() call. Attached a newer patch. Thanks. 0001-Create-a-new-session-for-postmaster-in-pg_ctl-by-cal.patch Description: Binary data
Re: [Patch] Create a new session in postmaster by calling setsid()
Paul Guo writes: > [ make the postmaster execute setsid() too ] I'm a bit skeptical of this proposal. Forcing the postmaster to dissociate from its controlling terminal is a good thing in some scenarios, but probably less good in others, and manual postmaster starts are probably mostly in the latter class. I wonder whether having "pg_ctl start" do a setsid() would accomplish the same result with less chance of breaking debugging usages. regards, tom lane
[Patch] Create a new session in postmaster by calling setsid()
Hello, Recently I encountered an issue during testing and rootcaused it as the title mentioned. postmaster children have done this (creating a new session) by calling InitPostmasterChild(), but postmaster itself does not. This could lead to some issues (e..g signal handling). The test script below is a simple example. $ cat test.sh pg_ctl -D ./data -l stop.log stop sleep 2 -- wait for pg down. pg_ctl -D ./data -l start.log start sleep 2 -- wait for pg up. echo "pg_sleep()-ing" psql -d postgres -c 'select pg_sleep(1000)' -- press ctrl+c, then you will see postmaster and its children are all gone. Long ago PG has pmdaemonize() to daemonize postmaster which of course create a new session. That code was removed in the patch below. commit f7ea6beaf4ca02b8e6dc576255e35a5b86035cb9 Author: Heikki Linnakangas Date: Mon Jul 4 14:35:44 2011 +0300 Remove silent_mode. You get the same functionality with "pg_ctl -l postmaster.log", or nohup. There was a small issue with LINUX_OOM_ADJ and silent_mode, namely that with silent_mode the postmaster process incorrectly used the OOM settings meant for backend processes. We certainly could've fixed that directly, but since silent_mode was redundant anyway, we might as well just remove it. Here is the related discussion about the patch. It seems that is related to oom score setting in fork_process(). https://www.postgresql.org/message-id/4E046EA5.8070101%40enterprisedb.com Personally I still like pg being a daemon, but probably I do not know some real cases which do not like daemon. If we do not go back to pgdaemonize() with further fix of fork_process() (assume I understand correctly) we could simply call setsid() in postmaster code to fix the issue above. I added a simple a patch as attached. Thanks. 0001-Create-a-new-session-in-postmaster-by-calling-setsid.patch Description: Binary data
