Horton

Thread

Commits

  1. Detach postmaster process from pg_ctl's session at server startup.

  2. Remove silent_mode. You get the same functionality with "pg_ctl -l

  1. [Patch] Create a new session in postmaster by calling setsid()

    Paul Guo <pguo@pivotal.io> — 2018-08-01T05:44:35Z

    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 <heikki.linnakangas@iki.fi>
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.

  2. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-02T14:30:16Z

    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

  3. Re: [Patch] Create a new session in postmaster by calling setsid()

    Paul Guo <pguo@pivotal.io> — 2018-08-06T04:11:26Z

    On Thu, Aug 2, 2018 at 10:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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
>

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.

  4. Re: [Patch] Create a new session in postmaster by calling setsid()

    Michael Paquier <michael@paquier.xyz> — 2018-09-12T06:56:39Z

    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

  5. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-12T14:50:25Z

    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

  6. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-12T18:35:05Z

    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

  7. Re: [Patch] Create a new session in postmaster by calling setsid()

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T19:36:22Z

    >>>>> "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)

  8. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-12T19:41:39Z

    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

  9. Re: [Patch] Create a new session in postmaster by calling setsid()

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-09-12T19:46:39Z

    >>>>> "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)

  10. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-12T19:55:00Z

    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

  11. Re: [Patch] Create a new session in postmaster by calling setsid()

    Michael Paquier <michael@paquier.xyz> — 2018-09-13T00:20:52Z

    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

  12. Re: [Patch] Create a new session in postmaster by calling setsid()

    Paul Guo <pguo@pivotal.io> — 2018-09-13T07:27:58Z

    On Thu, Sep 13, 2018 at 8:20 AM, Michael Paquier <michael@paquier.xyz>
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.

  13. Re: [Patch] Create a new session in postmaster by calling setsid()

    Andres Freund <andres@anarazel.de> — 2018-09-13T19:24:38Z

    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

  14. Re: [Patch] Create a new session in postmaster by calling setsid()

    Michael Paquier <michael@paquier.xyz> — 2018-09-30T12:47:54Z

    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

  15. Re: [Patch] Create a new session in postmaster by calling setsid()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-12-28T18:33:08Z

    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

  16. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-28T20:13:34Z

    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

  17. Re: [Patch] Create a new session in postmaster by calling setsid()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-12-30T19:59:29Z

    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

  18. Re: [Patch] Create a new session in postmaster by calling setsid()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2018-12-30T20:41:07Z

    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

  19. Re: [Patch] Create a new session in postmaster by calling setsid()

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-30T20:56:45Z

    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

  20. Re: [Patch] Create a new session in postmaster by calling setsid()

    Heikki Linnakangas <hlinnaka@iki.fi> — 2019-01-14T12:53:35Z

    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