Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Marco Atzeri]

On 18/01/2016 18:32, Andrew Dunstan wrote:



On 01/14/2016 12:38 AM, Michael Paquier wrote:

Hi all,

Beginning a new thread seems more adapted regarding $subject but
that's mentioned here as well:
http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com






Marco, as I think you do some packaging for Postgres in Cygwin, and
others, does that sound acceptable?





I think we can be a bit more adventurous and remove all the
Cygwin-specific code. See attached patch, which builds fine on buildfarm
cockatiel.

cheers

andrew



builds fine also here on 64bit


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Alvaro Herrera]

Andrew Dunstan wrote:

> I think we can be a bit more adventurous and remove all the Cygwin-specific
> code. See attached patch, which builds fine on buildfarm cockatiel.

Hopefully you also tested that it builds under MSVC :-)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Andrew Dunstan]

On 01/14/2016 12:38 AM, Michael Paquier wrote:

Hi all,

Beginning a new thread seems more adapted regarding $subject but
that's mentioned here as well:
http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com

It happens based on some investigation from Andrew that cygwin does
not need to use the service-related code, aka register/unregister
options or similar that are proper to WIN32 and rely instead on
cygrunsrv with a SYSV-like init file to manage the service. Based on
my tests with cygwin, I am able to see as well that a server started
within a cygwin session is able to persist even after it is closed,
which is kind of nice and I think that it is a additional reason to
not use the service-related code paths. Hence what about the following
patch, that makes cygwin behave like any *nix OS when using pg_ctl?
This simplifies a bit the code.

Marco, as I think you do some packaging for Postgres in Cygwin, and
others, does that sound acceptable?





I think we can be a bit more adventurous and remove all the 
Cygwin-specific code. See attached patch, which builds fine on buildfarm 
cockatiel.


cheers

andrew



diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 919d764..98aa1a0 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -39,13 +39,6 @@
 #include "getopt_long.h"
 #include "miscadmin.h"
 
-#if defined(__CYGWIN__)
-#include 
-#include 
-/* Cygwin defines WIN32 in windows.h, but we don't want it. */
-#undef WIN32
-#endif
-
 /* PID can be negative for standalone backend */
 typedef long pgpid_t;
 
@@ -105,7 +98,7 @@ static char backup_file[MAXPGPATH];
 static char recovery_file[MAXPGPATH];
 static char promote_file[MAXPGPATH];
 
-#if defined(WIN32) || defined(__CYGWIN__)
+#ifdef WIN32
 static DWORD pgctl_start_type = SERVICE_AUTO_START;
 static SERVICE_STATUS status;
 static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
@@ -133,7 +126,7 @@ static void do_kill(pgpid_t pid);
 static void print_msg(const char *msg);
 static void adjust_data_dir(void);
 
-#if defined(WIN32) || defined(__CYGWIN__)
+#ifdef WIN32
 #if (_MSC_VER >= 1800)
 #include 
 #else
@@ -165,7 +158,7 @@ static void unlimit_core_size(void);
 #endif
 
 
-#if defined(WIN32) || defined(__CYGWIN__)
+#ifdef WIN32
 static void
 write_eventlog(int level, const char *line)
 {
@@ -207,20 +200,11 @@ write_stderr(const char *fmt,...)
 	va_list		ap;
 
 	va_start(ap, fmt);
-#if !defined(WIN32) && !defined(__CYGWIN__)
+#ifndef WIN32
 	/* On Unix, we just fprintf to stderr */
 	vfprintf(stderr, fmt, ap);
 #else
 
-/*
- * On Cygwin, we don't yet have a reliable mechanism to detect when
- * we're being run as a service, so fall back to the old (and broken)
- * stderr test.
- */
-#ifdef __CYGWIN__
-#define	pgwin32_is_service()	(isatty(fileno(stderr)))
-#endif
-
 	/*
 	 * On Win32, we print to stderr if running on a console, or write to
 	 * eventlog if running as a service
@@ -718,7 +702,7 @@ test_postmaster_connection(pgpid_t pm_pid, bool do_checkpoint)
 #endif
 
 		/* No response, or startup still in process; wait */
-#if defined(WIN32)
+#ifdef WIN32
 		if (do_checkpoint)
 		{
 			/*
@@ -1342,7 +1326,7 @@ do_kill(pgpid_t pid)
 	}
 }
 
-#if defined(WIN32) || defined(__CYGWIN__)
+#ifdef WIN32
 
 #if (_MSC_VER < 1800)
 static bool
@@ -1408,20 +1392,6 @@ pgwin32_CommandLine(bool registration)
 		}
 	}
 
-#ifdef __CYGWIN__
-	/* need to convert to windows path */
-	{
-		char		buf[MAXPGPATH];
-
-#if CYGWIN_VERSION_DLL_MAJOR >= 1007
-		cygwin_conv_path(CCP_POSIX_TO_WIN_A, cmdPath, buf, sizeof(buf));
-#else
-		cygwin_conv_to_full_win32_path(cmdPath, buf);
-#endif
-		strcpy(cmdPath, buf);
-	}
-#endif
-
 	/* if path does not end in .exe, append it */
 	if (strlen(cmdPath) < 4 ||
 		pg_strcasecmp(cmdPath + strlen(cmdPath) - 4, ".exe") != 0)
@@ -1775,10 +1745,8 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
 	if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ALL_ACCESS, ))
 	{
 		/*
-		 * Most Windows targets make DWORD a 32-bit unsigned long.  Cygwin
-		 * x86_64, an LP64 target, makes it a 32-bit unsigned int.  In code
-		 * built for Cygwin as well as for native Windows targets, cast DWORD
-		 * before printing.
+		 * Most Windows targets make DWORD a 32-bit unsigned long, but
+		 * in case it doesn't cast DWORD before printing.
 		 */
 		write_stderr(_("%s: could not open process token: error code %lu\n"),
 	 progname, (unsigned long) GetLastError());
@@ -1819,10 +1787,6 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
 		return 0;
 	}
 
-#ifndef __CYGWIN__
-	AddUserToTokenDacl(restrictedToken);
-#endif
-
 	r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE, CREATE_SUSPENDED, NULL, NULL, , processInfo);
 
 	Kernel32Handle = LoadLibrary("KERNEL32.DLL");
@@ -1926,7 +1890,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
 	 */
 	return r;
 }

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Michael Paquier]

On Tue, Jan 19, 2016 at 2:32 AM, Andrew Dunstan  wrote:
>
>
> On 01/14/2016 12:38 AM, Michael Paquier wrote:
>>
>> Hi all,
>>
>> Beginning a new thread seems more adapted regarding $subject but
>> that's mentioned here as well:
>>
>> http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com
>>
>> It happens based on some investigation from Andrew that cygwin does
>> not need to use the service-related code, aka register/unregister
>> options or similar that are proper to WIN32 and rely instead on
>> cygrunsrv with a SYSV-like init file to manage the service. Based on
>> my tests with cygwin, I am able to see as well that a server started
>> within a cygwin session is able to persist even after it is closed,
>> which is kind of nice and I think that it is a additional reason to
>> not use the service-related code paths. Hence what about the following
>> patch, that makes cygwin behave like any *nix OS when using pg_ctl?
>> This simplifies a bit the code.
>>
>> Marco, as I think you do some packaging for Postgres in Cygwin, and
>> others, does that sound acceptable?
>>
>
>
>
> I think we can be a bit more adventurous and remove all the Cygwin-specific
> code. See attached patch, which builds fine on buildfarm cockatiel.

Ah, OK. I see the difference. It builds as well for me.

-#ifndef __CYGWIN__
-   AddUserToTokenDacl(restrictedToken);
-#endif
[...]
-#if defined(WIN32) || defined(__CYGWIN__)
+#ifdef WIN32
setvbuf(stderr, NULL, _IONBF, 0);
 #endif
Fine for me, those two do not seem to matter much as far as I have tested.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Andrew Dunstan]

On 01/18/2016 12:38 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:


I think we can be a bit more adventurous and remove all the Cygwin-specific
code. See attached patch, which builds fine on buildfarm cockatiel.

Hopefully you also tested that it builds under MSVC :-)




Why would I? This isn't having the slightest effect on MSVC builds.

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Andrew Dunstan]

On 01/18/2016 03:46 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:


On 01/18/2016 12:38 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:


I think we can be a bit more adventurous and remove all the Cygwin-specific
code. See attached patch, which builds fine on buildfarm cockatiel.

Hopefully you also tested that it builds under MSVC :-)

Why would I? This isn't having the slightest effect on MSVC builds.

You never know ... you might have inadvertently broken some WIN32 block.



If you can point out a line where that might be true I'll test it ;-)

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Alvaro Herrera]

Andrew Dunstan wrote:
> 
> 
> On 01/18/2016 12:38 PM, Alvaro Herrera wrote:
> >Andrew Dunstan wrote:
> >
> >>I think we can be a bit more adventurous and remove all the Cygwin-specific
> >>code. See attached patch, which builds fine on buildfarm cockatiel.
> >Hopefully you also tested that it builds under MSVC :-)
> 
> Why would I? This isn't having the slightest effect on MSVC builds.

You never know ... you might have inadvertently broken some WIN32 block.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Removing service-related code in pg_ctl for Cygwin

[Marco Atzeri]

On 14/01/2016 06:38, Michael Paquier wrote:

Hi all,

Beginning a new thread seems more adapted regarding $subject but
that's mentioned here as well:
http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com

It happens based on some investigation from Andrew that cygwin does
not need to use the service-related code, aka register/unregister
options or similar that are proper to WIN32 and rely instead on
cygrunsrv with a SYSV-like init file to manage the service. Based on
my tests with cygwin, I am able to see as well that a server started
within a cygwin session is able to persist even after it is closed,
which is kind of nice and I think that it is a additional reason to
not use the service-related code paths. Hence what about the following
patch, that makes cygwin behave like any *nix OS when using pg_ctl?
This simplifies a bit the code.

Marco, as I think you do some packaging for Postgres in Cygwin, and
others, does that sound acceptable?

Regards,



In general cygwin to behave like any *nix OS is the preferred solution.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Removing service-related code in pg_ctl for Cygwin

[Michael Paquier]

Hi all,

Beginning a new thread seems more adapted regarding $subject but
that's mentioned here as well:
http://www.postgresql.org/message-id/CAB7nPqQXghm_SdB5iniupz1atzMxk=95gv9a8ocdo83sxcn...@mail.gmail.com

It happens based on some investigation from Andrew that cygwin does
not need to use the service-related code, aka register/unregister
options or similar that are proper to WIN32 and rely instead on
cygrunsrv with a SYSV-like init file to manage the service. Based on
my tests with cygwin, I am able to see as well that a server started
within a cygwin session is able to persist even after it is closed,
which is kind of nice and I think that it is a additional reason to
not use the service-related code paths. Hence what about the following
patch, that makes cygwin behave like any *nix OS when using pg_ctl?
This simplifies a bit the code.

Marco, as I think you do some packaging for Postgres in Cygwin, and
others, does that sound acceptable?

Regards,
-- 
Michael


cygwin-removal-service-master.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers