Thread: PostgreSQL in Windows console and Ctrl-C
Hello all, when pg_ctl start is used to run PostgreSQL in a console window on Windows, it runs in the background (it is terminated by closing the window, but that is probably inevitable). There is one problem, however: The first Ctrl-C in that window, no matter in which situation, will cause the background postmaster to exit. If you, say, ping something, and press Ctrl-C to stop ping, you probably don't want the database to go away, too. The reason is that Windows delivers the Ctrl-C event to all processes using that console, not just to the foreground one. Here's a patch to fix that. "pg_ctl stop" still works, and it has no effect when running as a service, so it should be safe. It starts the postmaster in a new process group (similar to calling setpgrp() after fork()) that does not receive Ctrl-C events from the console window. -- Christian
Can someone with Windows expertise comment on whether this should be
applied?
---------------------------------------------------------------------------
On Tue, Jan 7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
> Hello all,
>
> when pg_ctl start is used to run PostgreSQL in a console window on
> Windows, it runs in the background (it is terminated by closing the
> window, but that is probably inevitable). There is one problem,
> however: The first Ctrl-C in that window, no matter in which
> situation, will cause the background postmaster to exit. If you,
> say, ping something, and press Ctrl-C to stop ping, you probably
> don't want the database to go away, too.
>
> The reason is that Windows delivers the Ctrl-C event to all
> processes using that console, not just to the foreground one.
>
> Here's a patch to fix that. "pg_ctl stop" still works, and it has no
> effect when running as a service, so it should be safe. It starts
> the postmaster in a new process group (similar to calling setpgrp()
> after fork()) that does not receive Ctrl-C events from the console
> window.
>
> --
> Christian
> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> new file mode 100644
> index 50d4586..89a9544
> *** a/src/bin/pg_ctl/pg_ctl.c
> --- b/src/bin/pg_ctl/pg_ctl.c
> *************** CreateRestrictedProcess(char *cmd, PROCE
> *** 1561,1566 ****
> --- 1561,1567 ----
> HANDLE restrictedToken;
> SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
> SID_AND_ATTRIBUTES dropSids[2];
> + DWORD flags;
>
> /* Functions loaded dynamically */
> __CreateRestrictedToken _CreateRestrictedToken = NULL;
> *************** CreateRestrictedProcess(char *cmd, PROCE
> *** 1636,1642 ****
> AddUserToTokenDacl(restrictedToken);
> #endif
>
> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, CREATE_SUSPENDED, NULL, NULL, &si,
processInfo);
>
> Kernel32Handle = LoadLibrary("KERNEL32.DLL");
> if (Kernel32Handle != NULL)
> --- 1637,1650 ----
> AddUserToTokenDacl(restrictedToken);
> #endif
>
> ! flags = CREATE_SUSPENDED;
> !
> ! /* Protect console process from Ctrl-C */
> ! if (!as_service) {
> ! flags |= CREATE_NEW_PROCESS_GROUP;
> ! }
> !
> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, flags, NULL, NULL, &si, processInfo);
>
> Kernel32Handle = LoadLibrary("KERNEL32.DLL");
> if (Kernel32Handle != NULL)
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +
On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
> Can someone with Windows expertise comment on whether this should be
> applied?
I tested the same in windows and it is working as specified.
The same background running server can be closed with ctrl+break command.
> ---------------------------------------------------------------------------
>
> On Tue, Jan 7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
>> Hello all,
>>
>> when pg_ctl start is used to run PostgreSQL in a console window on
>> Windows, it runs in the background (it is terminated by closing the
>> window, but that is probably inevitable). There is one problem,
>> however: The first Ctrl-C in that window, no matter in which
>> situation, will cause the background postmaster to exit. If you,
>> say, ping something, and press Ctrl-C to stop ping, you probably
>> don't want the database to go away, too.
>>
>> The reason is that Windows delivers the Ctrl-C event to all
>> processes using that console, not just to the foreground one.
>>
>> Here's a patch to fix that. "pg_ctl stop" still works, and it has no
>> effect when running as a service, so it should be safe. It starts
>> the postmaster in a new process group (similar to calling setpgrp()
>> after fork()) that does not receive Ctrl-C events from the console
>> window.
>>
>> --
>> Christian
>
>> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
>> new file mode 100644
>> index 50d4586..89a9544
>> *** a/src/bin/pg_ctl/pg_ctl.c
>> --- b/src/bin/pg_ctl/pg_ctl.c
>> *************** CreateRestrictedProcess(char *cmd, PROCE
>> *** 1561,1566 ****
>> --- 1561,1567 ----
>> HANDLE restrictedToken;
>> SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
>> SID_AND_ATTRIBUTES dropSids[2];
>> + DWORD flags;
>>
>> /* Functions loaded dynamically */
>> __CreateRestrictedToken _CreateRestrictedToken = NULL;
>> *************** CreateRestrictedProcess(char *cmd, PROCE
>> *** 1636,1642 ****
>> AddUserToTokenDacl(restrictedToken);
>> #endif
>>
>> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, CREATE_SUSPENDED, NULL, NULL, &si,
processInfo);
>>
>> Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>> if (Kernel32Handle != NULL)
>> --- 1637,1650 ----
>> AddUserToTokenDacl(restrictedToken);
>> #endif
>>
>> ! flags = CREATE_SUSPENDED;
>> !
>> ! /* Protect console process from Ctrl-C */
>> ! if (!as_service) {
>> ! flags |= CREATE_NEW_PROCESS_GROUP;
>> ! }
>> !
>> ! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, flags, NULL, NULL, &si, processInfo);
>>
>> Kernel32Handle = LoadLibrary("KERNEL32.DLL");
>> if (Kernel32Handle != NULL)
Regards,
Hari Babu
Fujitsu Australia
On Fri, Apr 11, 2014 at 11:58:58AM +1000, Haribabu Kommi wrote: > On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian <bruce@momjian.us> wrote: > > > > Can someone with Windows expertise comment on whether this should be > > applied? > > I tested the same in windows and it is working as specified. > The same background running server can be closed with ctrl+break command. OK. If I apply this, are you able to test to see if the problem is fixed? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Fri, Apr 11, 2014 at 12:12 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Fri, Apr 11, 2014 at 11:58:58AM +1000, Haribabu Kommi wrote: >> On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian <bruce@momjian.us> wrote: >> > >> > Can someone with Windows expertise comment on whether this should be >> > applied? >> >> I tested the same in windows and it is working as specified. >> The same background running server can be closed with ctrl+break command. > > OK. If I apply this, are you able to test to see if the problem is > fixed? I already tested in HEAD by applying the attached patch in the earlier mail. with ctrl+c command the background process is not closed. But with ctrl+break command the same can be closed. if this behavior is fine, then we can apply patch. Regards, Hari Babu Fujitsu Australia
On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
> Can someone with Windows expertise comment on whether this should be
> applied?
I don't think this is a complete fix, for example what about platform where
_CreateRestrictedToken() is not supported. For Example, the current
proposed fix will not work for below case:
if (_CreateRestrictedToken == NULL)
{
/*
* NT4 doesn't have CreateRestrictedToken, so just call ordinary
* CreateProcess
*/
write_stderr(_("%s: WARNING: cannot create restricted tokens on this
platform\n"), progname);
if (Advapi32Handle != NULL)
FreeLibrary(Advapi32Handle);
return CreateProcess(NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, &si,
processInfo);
}
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 04/11/2014 01:35 AM, Amit Kapila wrote:
> On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian <bruce@momjian.us> wrote:
>> Can someone with Windows expertise comment on whether this should be
>> applied?
> I don't think this is a complete fix, for example what about platform where
> _CreateRestrictedToken() is not supported. For Example, the current
> proposed fix will not work for below case:
>
> if (_CreateRestrictedToken == NULL)
> {
> /*
> * NT4 doesn't have CreateRestrictedToken, so just call ordinary
> * CreateProcess
> */
Are we really supporting NT4 any more? Even XP is about to be at end of
support from Microsoft.
cheers
andrew
On 2014-04-11 07:12:50 -0400, Andrew Dunstan wrote:
>
> On 04/11/2014 01:35 AM, Amit Kapila wrote:
> >On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian <bruce@momjian.us> wrote:
> >>Can someone with Windows expertise comment on whether this should be
> >>applied?
> >I don't think this is a complete fix, for example what about platform where
> >_CreateRestrictedToken() is not supported. For Example, the current
> >proposed fix will not work for below case:
> >
> >if (_CreateRestrictedToken == NULL)
> >{
> >/*
> >* NT4 doesn't have CreateRestrictedToken, so just call ordinary
> >* CreateProcess
> >*/
>
>
> Are we really supporting NT4 any more? Even XP is about to be at end of
> support from Microsoft.
I seem to recall that win2k was already desupported?
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
On Fri, Apr 11, 2014 at 4:42 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 04/11/2014 01:35 AM, Amit Kapila wrote:
>> I don't think this is a complete fix, for example what about platform
>> where
>> _CreateRestrictedToken() is not supported. For Example, the current
>> proposed fix will not work for below case:
>>
>> if (_CreateRestrictedToken == NULL)
>> {
>> /*
>> * NT4 doesn't have CreateRestrictedToken, so just call ordinary
>> * CreateProcess
>> */
> Are we really supporting NT4 any more? Even XP is about to be at end of
> support from Microsoft.
In Docs, it is mentioned as Windows (Win2000 SP4 and later).
Now what shall we do with this part of code, shall we keep it as it is and
just fix in other part of code or shall we remove this part of code?
Another thing to decide about this fix is that whether it is okay to fix it
for CTRL+C and leave the problem open for CTRL+BREAK?
(The current option used (CREATE_NEW_PROCESS_GROUP) will handle
only CTRL+C).
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
* From: Amit Kapila
> Another thing to decide about this fix is that whether it is okay to fix
> it for CTRL+C and leave the problem open for CTRL+BREAK?
> (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only
> CTRL+C).
I can think of three situations in which a postgres process can run on
Windows:
- single backend
- console background via pg_ctl
- service
The only way to deliver a console event to a service process is by
calling GenerateConsoleCtrlEvent() with that process( group)'s PID. If
anyone does that, they will know what they are doing, so we can disregard
that. The other two are tricky. In single-backend mode, we probably expect
both to work as usual (ending the process), while in the pg_ctl case,
they should both be ignored.
Ignoring Ctrl-C in the postmaster and all children is simple, this is what
my patch does. Ctrl-Break is more difficult to do. It is not limited to
the "foreground" process group, but is delivered to all processes attached
to the console that originates it. To ignore it, every process (postmaster,
backends, workers, etc.) will have to handle it in their own console
event handling function.
backend/port/win32/signal.c explicitly turns several of the console events,
including Ctrl-C and Ctrl-Break, into SIGINT. The simplest fix would be to
ignore Ctrl-Break there, effectively disabling it entirely under all
circumstances. I tried that, and it appears to work, but I don't know
enough about the signal emulation and the interactions between the various
processes to be sure this is the right place to do it. Single-backend mode
has no need for signal handling and does not use the emulation layer, so
it is unaffected.
Below is a new (right now very much proof-of-concept) patch to replace
my earlier one. It has the same effect on Ctrl-C the change to pg_ctl had,
and additionally ignores Ctrl-Break as well.
Please be gentle.
diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index 322b857..7ce5051 100644
--- a/src/backend/port/win32/signal.c
+++ b/src/backend/port/win32/signal.c
@@ -347,8 +347,12 @@ static BOOL WINAPIpg_console_handler(DWORD dwCtrlType){ if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT ||
- dwCtrlType == CTRL_CLOSE_EVENT ||
+ dwCtrlType == CTRL_BREAK_EVENT)
+ {
+ /* Ignore */
+ return TRUE;
+ }
+ else if (dwCtrlType == CTRL_CLOSE_EVENT || dwCtrlType == CTRL_SHUTDOWN_EVENT) {
pg_queue_signal(SIGINT);
--
Christian
On Sat, Apr 12, 2014 at 12:36 PM, Christian Ullrich <chris@chrullrich.net> wrote: > * From: Amit Kapila > >> Another thing to decide about this fix is that whether it is okay to fix >> it for CTRL+C and leave the problem open for CTRL+BREAK? >> (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only >> CTRL+C). > > Below is a new (right now very much proof-of-concept) patch to replace > my earlier one. It has the same effect on Ctrl-C the change to pg_ctl had, > and additionally ignores Ctrl-Break as well. This fix won't allow CTRL+C for other cases like when server is started directly with postgres binary. ./postgres -D <data_dir_path> I think this doesn't come under your original complaint and as such I don't see any problem in allowing CTRL+C for above case. One another way could be to use CREATE_NEW_CONSOLE instead of CREATE_NEW_PROCESS_GROUP in you previous fix, but I am not sure if it's acceptable to users to have a new console window for server. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
* From: Amit Kapila > On Sat, Apr 12, 2014 at 12:36 PM, Christian Ullrich <chris@chrullrich.net> > wrote: > > * From: Amit Kapila > > > >> Another thing to decide about this fix is that whether it is okay to > >> fix it for CTRL+C and leave the problem open for CTRL+BREAK? > >> (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only > >> CTRL+C). > > > > Below is a new (right now very much proof-of-concept) patch to replace > > my earlier one. It has the same effect on Ctrl-C the change to pg_ctl > > had, and additionally ignores Ctrl-Break as well. > > This fix won't allow CTRL+C for other cases like when server is started > directly with postgres binary. > ./postgres -D <data_dir_path> > I think this doesn't come under your original complaint and as such I > don't see any problem in allowing CTRL+C for above case. Good point. I had not thought of that case. Just how do you tell if your postmaster was started by pg_ctl or directly on the command line? - pg_ctl starts the postmaster through an intervening shell, so even if it did not exit right after that, we could not checkthe parent process name (assuming nobody renamed pg_ctl). - pg_ctl starts the postmaster with stdin redirected from the null device, but so could anyone else. The same is true foraccess rights, token groups, and most everything else pg_ctl does. - I don't want to add a new command line flag to postgres.exe just to tell it who its parent is. - Job objects may not be supported on the underlying OS, or creation may have failed, or the interactive console may havebeen running in one already, so the sheer existence of a job is no proof it's ours. There are some possible solutions: - pg_ctl could set an environment variable (unless it has to be compatible with postmasters from different versions, andit does not, does it?). pg_ctl creates a named job object, and the postmaster has all the information it needs to reconstruct the name, so it cancheck if it is running inside that pg_ctl-created job. I would appreciate some advice on this. > One another way could be to use CREATE_NEW_CONSOLE instead of > CREATE_NEW_PROCESS_GROUP in you previous fix, but I am not sure if it's > acceptable to users to have a new console window for server. It might. That would also isolate stderr logging from whatever else the user is doing in the window, and it would work correctly in the pg_ctl (and by extension the direct-postmaster) case. It would not do anything for events generated by keystrokes in the new console window, though. There would also have to be a command line option to pg_ctl to either enable or disable it, not sure which. -- Christian
On Sun, Apr 13, 2014 at 5:59 PM, Christian Ullrich <chris@chrullrich.net> wrote: > There are some possible solutions: > > - pg_ctl could set an environment variable (unless it has to be compatible > with postmasters from different versions, and it does not, does it?). Do you mean to say use some existing environment variable? Introducing an environment variable to solve this issue or infact using some existing environ variable doesn't seem to be the best way to pass such information. > pg_ctl creates a named job object, and the postmaster has all the > information it needs to reconstruct the name, so it can check if it is > running inside that pg_ctl-created job. We are already creating one job object, so may be that itself can be used, but not sure if Job Objects are supported on all platforms on which postgres works. If you have to pass such information, then why don't pass some special flag in command to CreateProcess()? There is always a chance that we ignore the CTRL+C for some app which we don't intend if we solve the problem by passing some info, as some other app could also do so, but I think it should not be a big problem. > I would appreciate some advice on this. > >> One another way could be to use CREATE_NEW_CONSOLE instead of >> CREATE_NEW_PROCESS_GROUP in you previous fix, but I am not sure if it's >> acceptable to users to have a new console window for server. > > It might. That would also isolate stderr logging from whatever else the > user is doing in the window, and it would work correctly in the pg_ctl > (and by extension the direct-postmaster) case. It would not do anything > for events generated by keystrokes in the new console window, though. > > There would also have to be a command line option to pg_ctl to either > enable or disable it, not sure which. The problem can be solved this way, but the only question here is whether it is acceptable for users to have a new console window for server. Can others also please share their opinion if this fix (start server in new console) seems acceptable or shall we try by passing some information from pg_ctl and then ignore CTRL+C && CTRL+BREAK for it? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
* From: Amit Kapila > On Sun, Apr 13, 2014 at 5:59 PM, Christian Ullrich <chris@chrullrich.net> > wrote: > > There are some possible solutions: > > > > - pg_ctl could set an environment variable (unless it has to be > > compatible with postmasters from different versions, and it does > > not, does it?). > > Do you mean to say use some existing environment variable? > Introducing an environment variable to solve this issue or infact using > some existing environ variable doesn't seem to be the best way to pass > such information. I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was set, the postmaster etc. would ignore the events. > > pg_ctl creates a named job object, and the postmaster has all the > > information it needs to reconstruct the name, so it can check if it is > > running inside that pg_ctl-created job. > > We are already creating one job object, so may be that itself can be > used, but not sure if Job Objects are supported on all platforms on which > postgres works. I mentioned that in my earlier message; we cannot rely on having that job object around. If the OS does not support them (not much of a problem nowadays, I think; anything anyone is going to run the version this will first be released in on is going to) or, more likely, if something uses PostgreSQL as "embedded" database and starts it in a job of their own, pg_ctl will not create its job object. There may also be software out there that a) runs PostgreSQL in this way, b) uses the console events to stop it again, and c) does that with or without a separate job object. Hence, deciding based on the existence of a job object would break backward compatibility in this case, assuming it exists in reality. > If you have to pass such information, then why don't pass some special > flag in command to CreateProcess()? Because I wanted to avoid introducing a command line option to tell the postmaster who started it. If we cannot look at job objects, and an environment variable is not an option either, then this looks like the best remaining solution. I will create a patch based on this approach. > There is always a chance that we ignore the CTRL+C for some app which > we don't intend if we solve the problem by passing some info, as some > other app could also do so, but I think it should not be a big problem. No. In fact, if whatever starts the postmaster does pass that flag, we can safely assume that it did not do so just for fun. -- Christian
On Mon, Apr 14, 2014 at 09:34:14AM +0530, Amit Kapila wrote: > The problem can be solved this way, but the only question here is whether > it is acceptable for users to have a new console window for server. > > Can others also please share their opinion if this fix (start server in new > console) seems acceptable or shall we try by passing some information > from pg_ctl and then ignore CTRL+C && CTRL+BREAK for it? I wanted to point out that I think this patch is only appropriate for head, not backpatching. Also, on Unix we have to handle signals that come from the kill command. Can you send CTRL+C from other applications on Windows? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, Apr 14, 2014 at 11:46 AM, Christian Ullrich <chris@chrullrich.net> wrote: > * From: Amit Kapila >> Do you mean to say use some existing environment variable? >> Introducing an environment variable to solve this issue or infact using >> some existing environ variable doesn't seem to be the best way to pass >> such information. > > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was set, > the postmaster etc. would ignore the events. Do you plan to reset it and if yes when? I think there is chance that after setting this environment variable, some other instance of server (postmaster) can read it and missed the signal which it should have otherwise processed. Irrespective of above problem, I think using environment variable might not be a good way to solve this problem. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Apr 15, 2014 at 4:21 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Mon, Apr 14, 2014 at 09:34:14AM +0530, Amit Kapila wrote: >> The problem can be solved this way, but the only question here is whether >> it is acceptable for users to have a new console window for server. >> >> Can others also please share their opinion if this fix (start server in new >> console) seems acceptable or shall we try by passing some information >> from pg_ctl and then ignore CTRL+C && CTRL+BREAK for it? > > I wanted to point out that I think this patch is only appropriate for > head, not backpatching. Also, on Unix we have to handle signals that > come from the kill command. Can you send CTRL+C from other > applications on Windows? I think there are ways to achieve it, but might not be very straightforward. If we start server in new console window through pg_ctl, then we *don't* need to change any signal handler to catch and do something specific for CTRL+C/CTRL+BREAK. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Mon, Apr 14, 2014 at 2:16 AM, Christian Ullrich <chris@chrullrich.net> wrote: > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was set, > the postmaster etc. would ignore the events. Why not just pass a command-line switch? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* From: Robert Haas > On Mon, Apr 14, 2014 at 2:16 AM, Christian Ullrich > <chris@chrullrich.net> wrote: > > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was > > set, the postmaster etc. would ignore the events. > > Why not just pass a command-line switch? Because, as I wrote in the message you are quoting, I did not think that having a command-line option for the sole purpose of telling the postmaster who its parent is was a suitable solution. I had already given up on that idea based on Amit's advice, and I will create a patch based on a command-line option. While I have you here, though, any suggestions on what the name of that option should be? I think --background is about right. Also, how should I treat the option on non-Windows platforms? Should it just not be there (= error), or be ignored if present? -- Christian
* From: Bruce Momjian > On Mon, Apr 14, 2014 at 09:34:14AM +0530, Amit Kapila wrote: > > The problem can be solved this way, but the only question here is > > whether it is acceptable for users to have a new console window for > > server. > > > > Can others also please share their opinion if this fix (start server > > in new console) seems acceptable or shall we try by passing some > > information from pg_ctl and then ignore CTRL+C && CTRL+BREAK for it? > > I wanted to point out that I think this patch is only appropriate for > head, not backpatching. Also, on Unix we have to handle signals that Yes, of course. > come from the kill command. Can you send CTRL+C from other applications > on Windows? Yes again, using GenerateConsoleCtrlEvent() you can send these events to any (console-attached) process you have the required permissions for, but that is not an issue for the same reason it isn't one on Unix. All the target process sees is the event, it cannot determine (nor does it care) where the event came from. -- Christian
* From: Amit Kapila > On Mon, Apr 14, 2014 at 11:46 AM, Christian Ullrich > <chris@chrullrich.net> wrote: > > * From: Amit Kapila > >> Do you mean to say use some existing environment variable? > >> Introducing an environment variable to solve this issue or infact > >> using some existing environ variable doesn't seem to be the best way > >> to pass such information. > > > > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was > > set, the postmaster etc. would ignore the events. > > Do you plan to reset it and if yes when? > I think there is chance that after setting this environment variable, > some other instance of server (postmaster) can read it and missed the > signal which it should have otherwise processed. We have decided not to go this way, but just for completeness: Environment inheritance works the same way on Windows as on Unix. When a process is started with a modified environment (one of the plentiful arguments of CreateProcess() et al.), only that process and its descendants see the modification. I had not planned to set a system-level or user-level variable. -- Christian
On Tue, Apr 15, 2014 at 11:53 PM, Christian Ullrich <chris@chrullrich.net> wrote: > * From: Robert Haas >> Why not just pass a command-line switch? > > Because, as I wrote in the message you are quoting, I did not think that > having a command-line option for the sole purpose of telling the > postmaster who its parent is was a suitable solution. > > I had already given up on that idea based on Amit's advice, and I will > create a patch based on a command-line option. > > While I have you here, though, any suggestions on what the name of that > option should be? I think --background is about right. --background as switch name seems to be okay. > Also, how should > I treat the option on non-Windows platforms? Should it just not be there > (= error), or be ignored if present? I think ignored for non-windows is better way to proceed. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Tue, Apr 15, 2014 at 2:23 PM, Christian Ullrich <chris@chrullrich.net> wrote: > * From: Robert Haas >> On Mon, Apr 14, 2014 at 2:16 AM, Christian Ullrich >> <chris@chrullrich.net> wrote: > >> > I meant creating a new one, yes. If, say, PGSQL_BACKGROUND_JOB was >> > set, the postmaster etc. would ignore the events. >> >> Why not just pass a command-line switch? > > Because, as I wrote in the message you are quoting, I did not think that > having a command-line option for the sole purpose of telling the > postmaster who its parent is was a suitable solution. True, but you didn't say why, which is what I asked. You just said you didn't think it was a good idea, without elaborating. > While I have you here, though, any suggestions on what the name of that > option should be? I think --background is about right. Also, how should > I treat the option on non-Windows platforms? Should it just not be there > (= error), or be ignored if present? Well, we had a recent discussion that's related to this, about a not-entirely-dissimilar problem on Solaris: http://www.postgresql.org/message-id/22636.1392419221@sss.pgh.pa.us The proposal there was --daemonize. That seems somehow inapposite for Windows, though. I don't really have a strong opinion at this point on what the best naming is; I don't love --background, but I haven't got a better idea, either. On the topic of how the option should be handled on non-Windows platforms, I guess I'd vote for accepting it and ignoring it. The Solaris issue can (and, IMHO, should) be fixed by teaching pg_ctl to use fork()+exec()+setsid() rather than system()+hope the shell behaves the way we'd like, so we don't currently have an apparent need for any special handling for this case on any platform other than Windows. But that could change in the future, so I think it'd be smart to choose a somewhat generic option name and just define it as a no-op on non-Windows platforms for now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
OK, here is the first draft against current master. It builds on Windows with VS 2012 and on FreeBSD 10 with clang 3.3. I ran the regression tests on Windows, they all pass. The changed behavior is limited to Windows, where it now silently ignores Ctrl-C and Ctrl-Break when started via pg_ctl start. I don't think there is currently any support for switch-type long options, so rather than invent my own, I squeezed the two lines I added into postmaster.c where they fit best; unfortunately, the result is quite ugly. I'll be happy to refine that if someone can give me a hint on how to do it. Patch attached, will add to CF soon. -- Christian
From: "Christian Ullrich" <chris@chrullrich.net>
> OK, here is the first draft against current master. It builds on Windows
> with VS 2012 and on FreeBSD 10 with clang 3.3. I ran the regression
> tests on Windows, they all pass.
>
> The changed behavior is limited to Windows, where it now silently
> ignores Ctrl-C and Ctrl-Break when started via pg_ctl start.
>
> I don't think there is currently any support for switch-type long
> options, so rather than invent my own, I squeezed the two lines I added
> into postmaster.c where they fit best; unfortunately, the result is
> quite ugly. I'll be happy to refine that if someone can give me a hint
> on how to do it.
Overall, the patch seems good as it is based on the discussion. I found a
few problems:
(1)
The patch doesn't apply to HEAD. Could you rebase your patch?
patching file src/bin/pg_ctl/pg_ctl.c
Hunk #1 FAILED at 453.
1 out of 1 hunk FAILED -- saving rejects to file src/bin/pg_ctl/pg_ctl.c.rej
(2)
Although I haven't tried, doesn't pg_ctl start fail on non-Windows platforms
because of the following check?
! if (opt == '-')
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("--%s requires a value",
! optarg)));
And, in the postgres reference page,
http://www.postgresql.org/docs/devel/static/app-postgres.html
there's a paragraph:
"The -- options will not work on FreeBSD or OpenBSD. Use -c instead. This is
a bug in the affected operating systems; a future release of PostgreSQL will
provide a workaround if this is not fixed."
Would --background work on FreeBSD and OpenBSD (i.e. would pg_ctl start
succeed)? I don't have access to those platforms.
(3)
--background will also be used by restart subcommand, won't it?
+ in a console window. It is used automatically by
+ <command>pg_ctl</command> when called with the
+ <option>start</option> subcommand.
Regards
MauMau
* From: MauMau [mailto:maumau307@gmail.com]
> From: "Christian Ullrich" <chris@chrullrich.net>
> > OK, here is the first draft against current master. It builds on Windows
> > with VS 2012 and on FreeBSD 10 with clang 3.3. I ran the regression
> > tests on Windows, they all pass.
> >
> > The changed behavior is limited to Windows, where it now silently
> > ignores Ctrl-C and Ctrl-Break when started via pg_ctl start.
> >
> > I don't think there is currently any support for switch-type long
> > options, so rather than invent my own, I squeezed the two lines I added
> > into postmaster.c where they fit best; unfortunately, the result is
> > quite ugly. I'll be happy to refine that if someone can give me a hint
> > on how to do it.
>
> Overall, the patch seems good as it is based on the discussion. I found a
> few problems:
Thank you for the review. I will rebase, retest, and resubmit as soon as I find the time, certainly sometime this
week.
> (2)
> Although I haven't tried, doesn't pg_ctl start fail on non-Windows
> platforms
> because of the following check?
>
> ! if (opt == '-')
> ! ereport(ERROR,
> ! (errcode(ERRCODE_SYNTAX_ERROR),
> ! errmsg("--%s requires a value",
> ! optarg)));
On non-Windows platforms, the --background option is not passed, and the option handling is unmodified except for an
additionalpair of braces. The postmaster does not pass the option to its children on any platform.
> And, in the postgres reference page,
>
> http://www.postgresql.org/docs/devel/static/app-postgres.html
>
> there's a paragraph:
>
> "The -- options will not work on FreeBSD or OpenBSD. Use -c instead. This
> is a bug in the affected operating systems; a future release of PostgreSQL
> willprovide a workaround if this is not fixed."
>
> Would --background work on FreeBSD and OpenBSD (i.e. would pg_ctl start
> succeed)? I don't have access to those platforms.
pg_ctl does not pass the option anywhere but on Windows, and postmaster.c does not recognize it anywhere else. If it is
encounteredon a platform where it does not make sense, it will be treated like any other (unknown) long option.
This is actually the weakest point of the existing patch, in my opinion. Jamming the long option handling into
postmaster.cby way of #ifdef WIN32 feels wrong, but I could not figure out a better way to do it.
> (3)
> --background will also be used by restart subcommand, won't it?
>
> + in a console window. It is used automatically by
> + <command>pg_ctl</command> when called with the
> + <option>start</option> subcommand.
Restart is implemented as stop/start, so, yes.
--
Christian
From: "Christian Ullrich" <chris@chrullrich.net> > On non-Windows platforms, the --background option is not passed, and the > option handling is unmodified except for an additional pair of braces. The > postmaster does not pass the option to its children on any platform. > pg_ctl does not pass the option anywhere but on Windows, and postmaster.c > does not recognize it anywhere else. If it is encountered on a platform > where it does not make sense, it will be treated like any other (unknown) > long option. OK. > Restart is implemented as stop/start, so, yes. Then, please mention restart mode as well like "start and restart mode" for clarification. Regards MauMau
On Tue, Jun 24, 2014 at 09:24:43AM +0000, Christian Ullrich wrote: > pg_ctl does not pass the option anywhere but on Windows, and postmaster.c does not recognize it anywhere else. If it isencountered on a platform where it does not make sense, it will be treated like any other (unknown) long option. > > This is actually the weakest point of the existing patch, in my opinion. Jamming the long option handling into postmaster.cby way of #ifdef WIN32 feels wrong, but I could not figure out a better way to do it. I liked the proposal here; was there a problem with it? http://www.postgresql.org/message-id/CA+TgmoZ3aKE4EnCTmQmZSyKC_0pjL_u4C_x47GE48uY1upBNxg@mail.gmail.com The pg_upgrade test suite and the $(prove_check)-based test suites rely on their pg_ctl-started postmasters receiving any console ^C. pg_ctl deserves a --foreground or --no-background option for callers that prefer the current behavior. That, or those tests need a new way to launch the postmaster. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
* From: Noah Misch [mailto:noah@leadboat.com] > I liked the proposal here; was there a problem with it? > http://www.postgresql.org/message- > id/CA+TgmoZ3aKE4EnCTmQmZSyKC_0pjL_u4C_x47GE48uY1upBNxg@mail.gmail.com You're referring to the suggestion of accepting and ignoring the option on non-Windows, right? I can do that, I just don't see the point as long as pg_ctl has a separate code path (well, #ifdef) for Windows anyway. > The pg_upgrade test suite and the $(prove_check)-based test suites rely on > their pg_ctl-started postmasters receiving any console ^C. pg_ctl > deserves a --foreground or --no-background option for callers that prefer > the current behavior. That, or those tests need a new way to launch the > postmaster. Ah. More good news. Just to confirm, this is only about the tests, right, not the code they are testing? If so, is there even a way to run either on Windows? The pg_upgrade test suite is a shell script, and prove_check is defined in Makefile.global and definitely not applicable to Windows. -- Christian
On Mon, Jun 30, 2014 at 07:28:03PM +0000, Christian Ullrich wrote: > * From: Noah Misch [mailto:noah@leadboat.com] > > > I liked the proposal here; was there a problem with it? > > http://www.postgresql.org/message- > > id/CA+TgmoZ3aKE4EnCTmQmZSyKC_0pjL_u4C_x47GE48uY1upBNxg@mail.gmail.com > > You're referring to the suggestion of accepting and ignoring the option on > non-Windows, right? I can do that, I just don't see the point as long as > pg_ctl has a separate code path (well, #ifdef) for Windows anyway. Yes. We normally recognize platform-specific options on every platform. For example, the effective_io_concurrency GUC exists on all platforms, but you can only change it on platforms where it matters. In that light, one could argue for raising an error for --background on non-Windows systems. I don't have a strong opinion on raising an error vs. ignoring the option, but I do think the outcome of --background should be distinguishable from the outcome of --sometypo on all platforms. > > The pg_upgrade test suite and the $(prove_check)-based test suites rely on > > their pg_ctl-started postmasters receiving any console ^C. pg_ctl > > deserves a --foreground or --no-background option for callers that prefer > > the current behavior. That, or those tests need a new way to launch the > > postmaster. > > Ah. More good news. Just to confirm, this is only about the tests, right, > not the code they are testing? Yes; the consequence of ignoring ^C is that the test postmaster would persist indefinitely after the ^C. The system under test doesn't care per se, but future test runs might fail strangely due to the conflicting postmaster. The user running the tests won't appreciate the clutter in his process list. > If so, is there even a way to run either on Windows? The pg_upgrade test > suite is a shell script, and prove_check is defined in Makefile.global and > definitely not applicable to Windows. contrib/pg_upgrade/test.sh works under MSYS. Perhaps $(prove_check) currently doesn't, but that's something to be fixed, not relied upon. There's a clone of contrib/pg_upgrade/test.sh in src/tools/msvc/vcregress.pl, and I expect it would have the same problem. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
* From: Noah Misch [mailto:noah@leadboat.com] > On Mon, Jun 30, 2014 at 07:28:03PM +0000, Christian Ullrich wrote: > > * From: Noah Misch [mailto:noah@leadboat.com] > > > I liked the proposal here; was there a problem with it? > > > http://www.postgresql.org/message- > > > id/CA+TgmoZ3aKE4EnCTmQmZSyKC_0pjL_u4C_x47GE48uY1upBNxg@mail.gmail.co > > > m > > > > You're referring to the suggestion of accepting and ignoring the > > option on non-Windows, right? I can do that, I just don't see the > > point as long as pg_ctl has a separate code path (well, #ifdef) for > > Windows anyway. > > Yes. We normally recognize platform-specific options on every platform. > For example, the effective_io_concurrency GUC exists on all platforms, but > you can only change it on platforms where it matters. In that light, one > could argue for raising an error for --background on non-Windows systems. > I don't have a strong opinion on raising an error vs. ignoring the option, > but I do think the outcome of --background should be distinguishable from > the outcome of --sometypo on all platforms. I'm convinced, will change to --sometypo treatment. > > > The pg_upgrade test suite and the $(prove_check)-based test suites > > > rely on their pg_ctl-started postmasters receiving any console ^C. > > > pg_ctl deserves a --foreground or --no-background option for callers > > > that prefer the current behavior. That, or those tests need a new > > > way to launch the postmaster. > > > > Ah. More good news. Just to confirm, this is only about the tests, > > right, not the code they are testing? > > Yes; the consequence of ignoring ^C is that the test postmaster would > persist indefinitely after the ^C. The system under test doesn't care per No it won't, please see below. > > If so, is there even a way to run either on Windows? The pg_upgrade > > test suite is a shell script, and prove_check is defined in > > Makefile.global and definitely not applicable to Windows. > > contrib/pg_upgrade/test.sh works under MSYS. Perhaps $(prove_check) > currently doesn't, but that's something to be fixed, not relied upon. Yes. That doesn't matter, though. > There's a clone of contrib/pg_upgrade/test.sh in > src/tools/msvc/vcregress.pl, and I expect it would have the same problem. Oh, right. I didn't notice that because it doesn't mention upgradecheck in its usage message. I think I know where we're talking past each other. You may be assuming that kill(postmaster, SIGINT) would be affected bymy patch. It would not. PostgreSQL's signal emulation on Windows works completely separately from the actual Windows analogyto signals in console processes. My patch drops these "analogous" events, but the emulated signals still work thesame way as before. When Ctrl-C is pressed in a console window, pg_console_handler() in backend/port/win32/signal.c is called with a CTRL_C_EVENT,and converts that to an emulated SIGINT (unless I tell it not to). That emulated SIGINT is then processed asusual. pg_ctl -m fast stop sends the emulated SIGINT directly. Anyway, I just ran upgradecheck, and it reported success without leaving any stale postmasters around. -- Christian
On Mon, Jun 30, 2014 at 09:16:45PM +0000, Christian Ullrich wrote: > * From: Noah Misch [mailto:noah@leadboat.com] > > Yes; the consequence of ignoring ^C is that the test postmaster would > > persist indefinitely after the ^C. The system under test doesn't care per > > No it won't, please see below. > I think I know where we're talking past each other. You may be assuming that kill(postmaster, SIGINT) would be affectedby my patch. It would not. PostgreSQL's signal emulation on Windows works completely separately from the actual Windowsanalogy to signals in console processes. My patch drops these "analogous" events, but the emulated signals still workthe same way as before. I wasn't assuming otherwise. > When Ctrl-C is pressed in a console window, pg_console_handler() in backend/port/win32/signal.c is called with a CTRL_C_EVENT,and converts that to an emulated SIGINT (unless I tell it not to). That emulated SIGINT is then processed asusual. pg_ctl -m fast stop sends the emulated SIGINT directly. The main point of this patch is to have pg_console_handler() in the postmaster ignore ^C when "pg_ctl start" had started this postmaster. Right? > Anyway, I just ran upgradecheck, and it reported success without leaving any stale postmasters around. By the time the test reports success, it has already shutdown the postmaster cleanly. I'm focusing on the case where the user decides halfway through the test run to cancel it. Currently, ^C will reach both the test driver and the postmaster, and everything will shut down; no "pg_ctl stop" is involved. What happens if you issue "vcregress upgradecheck" and strike ^C between the "Setting up data for upgrading" and "Dumping old cluster" messages? -- Noah Misch EnterpriseDB http://www.enterprisedb.com