Обсуждение: [Patch] Create a new session in postmaster by calling setsid()

Paul Guo <pguo@pivotal.io> 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

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

Michael Paquier <michael@paquier.xyz> 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

I wrote:
> Michael Paquier <michael@paquier.xyz> 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

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> 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)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> 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

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> 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)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> 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

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

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

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

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

Heikki Linnakangas <hlinnaka@iki.fi> 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

On 28/12/2018 22:13, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> 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. 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.

Looking at the existing docs in the "Starting the Database Server" 
section, and the pg_ctl reference page, I don't think we need to change 
anything. This is the same user-visible behavior as before, except for 
the case with a script like in Paul's original post. And that is almost 
a bug fix: the manual says that pg_ctl "will start the server in the 
background", and I think the new behavior matches that description 
better than the old one. Perhaps we should mention something about how 
CTRL-C will interrupt the server launch in "pg_ctl -w" mode, but that 
isn't new, either.

I don't think we should backpatch this. We haven't heard any complaints 
about this until now, and it seems plausible, although unlikely, that 
someone might be relying on the current behavior.

- Heikki

On 30/12/2018 21:59, Heikki Linnakangas wrote:
> On 28/12/2018 22:13, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> 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

Heikki Linnakangas <hlinnaka@iki.fi> 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

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