PostgreSQL Service on Windows does not start if data directory given is relative path

Started by Rajeev rastogiabout 12 years ago3 messages

rajeev.rastogi@huawei.com

about 12 years ago

1 attachment(s)

I have found a case that PostgreSQL as win32 service does not start, if the data directory given as relative path.
Error observed in this case is:
"The PostgreSQL on Local Computer started and then stopped".

This may happen because relative path given will be relative to path from where service is registered but
the path from where WIN32 service execution gets invoked may be different and hence it won't be able
to find the data directory.

I have fixed the same by internally converting the relative path to absolute path as it is being done for executable file.

Attached is the patch with the fix.
Please provide your opinion.

I will add this to Jan 2014 CommitFest.

Thanks and Regards,
Kumar Rajeev Rastogi

Attachments:

pgctl_win32service_rel_dbpath.patchapplication/octet-stream; name=pgctl_win32service_rel_dbpath.patchDownload

*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 1243,1248 **** pgwin32_CommandLine(bool registration)
--- 1243,1249 ----
  	PQExpBuffer cmdLine = createPQExpBuffer();
  	char		cmdPath[MAXPGPATH];
  	int			ret;
+ 	char		dbPath[MAXPGPATH];
  
  	if (registration)
  	{
***************
*** 1297,1303 **** pgwin32_CommandLine(bool registration)
  						  register_servicename);
  
  	if (pg_config)
! 		appendPQExpBuffer(cmdLine, " -D \"%s\"", pg_config);
  
  	if (registration && do_wait)
  		appendPQExpBuffer(cmdLine, " -w");
--- 1298,1313 ----
  						  register_servicename);
  
  	if (pg_config)
! 	{
! 		if (find_my_abs_path(pg_config, dbPath) != 0)
! 		{
! 			write_stderr(_("%s: could not identify current directory\n"),
! 						 progname);
! 			exit(1);
! 		}
! 
! 		appendPQExpBuffer(cmdLine, " -D \"%s\"", dbPath);
! 	}
  
  	if (registration && do_wait)
  		appendPQExpBuffer(cmdLine, " -w");
*** a/src/common/exec.c
--- b/src/common/exec.c
***************
*** 586,591 **** set_pglocale_pgservice(const char *argv0, const char *app)
--- 586,628 ----
  	}
  }
  
+ /*
+  * find_my_abs_path -- find an absolute path to a given file/path
+  *
+  *	inpath is the input path for which absolute path required.
+  *	retpath is the output area (must be of size MAXPGPATH)
+  *	Returns 0 if OK, -1 if error.
+  */
+ int
+ find_my_abs_path(char *inpath, char *retpath)
+ {
+ 	char		cwd[MAXPGPATH];
+ 
+ 	/*
+ 	 * Check if it is already absolute path.
+ 	 */
+ 	if (is_absolute_path(inpath))
+ 	{
+ 		StrNCpy(retpath, inpath, MAXPGPATH);
+ 	}
+ 	else
+ 	{
+ 		if (!getcwd(cwd, MAXPGPATH))
+ 		{
+ 			log_error(_("could not identify current directory: %s"),
+ 					  strerror(errno));
+ 			return -1;
+ 		}
+ 
+ 		join_path_components(retpath, cwd, inpath);
+ 		canonicalize_path(retpath);
+ 	}
+ 
+ 	/* use backslashes in path to avoid problems with some third-party tools */
+ 	make_native_path(retpath);
+ 	return resolve_symlinks(retpath);
+ }
+ 
  #ifdef WIN32
  
  /*
*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 95,100 **** extern void set_pglocale_pgservice(const char *argv0, const char *app);
--- 95,101 ----
  extern int	find_my_exec(const char *argv0, char *retpath);
  extern int find_other_exec(const char *argv0, const char *target,
  				const char *versionstr, char *retpath);
+ extern int	find_my_abs_path(char *inpath, char *retpath);
  
  /* Windows security token manipulation (in exec.c) */
  #ifdef WIN32

David Rowley

dgrowleyml@gmail.com

about 12 years ago

In reply to: Rajeev rastogi (#1)

Re: PostgreSQL Service on Windows does not start if data directory given is relative path

On Tue, Jan 7, 2014 at 12:42 AM, Rajeev rastogi
<rajeev.rastogi@huawei.com>wrote:

I have found a case that PostgreSQL as win32 service does not start, if
the data directory given as relative path.

Error observed in this case is:

“The PostgreSQL on Local Computer started
and then stopped”.

This may happen because relative path given will be relative to path from
where service is registered but

the path from where WIN32 service execution gets invoked may be different
and hence it won’t be able

to find the data directory.

I have fixed the same by internally converting the relative path to
absolute path as it is being done for executable file.

Attached is the patch with the fix.

Please provide your opinion.

Hi,

I've not quite got around to testing this yet, but I think the patch is a
good idea as I can see that I can use a relative path when I start
postgres.exe from the command line, I guess the behaviour should likely be
the same when installed as a windows service.

So, I've just been looking over this patch and I'm just wondering about a
few things:

In pgwin32_CommandLine you declare dbPath, it looks like you could declare
this in the scope of; if (pg_config)

In find_my_abs_path you're making a call to StrNCpy, I know you likely just
used find_my_exec as an example here, but I'd say the use of StrNCpy is not
really needed here and is a bit of a waste of cycles. I'd rather see
strlcpy being used as strncpy will needlessly zero the remaining buffer
space.

Also in find_my_abs_path, I'm just wondering if the cwd variable is really
needed, I think you could just use retpath each time and it would also save
a little bit of copying that's done in join_path_components(). By the looks
of it you can just call join_path_components(retpath, retpath, inpath).
Perhaps some people would disagree, but I'm not really seeing the harm in
it and it saves some copying too.

Regards

David Rowley

Show quoted text

I will add this to Jan 2014 CommitFest.

*Thanks and Regards,*

*Kumar Rajeev Rastogi *

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

Rajeev rastogi

rajeev.rastogi@huawei.com

almost 12 years ago

In reply to: David Rowley (#2)

1 attachment(s)

Re: PostgreSQL Service on Windows does not start if data directory given is relative path

On Tue, Jan 12, 2014 David Rowley wrote:

I have found a case that PostgreSQL as win32 service does not start, if the data directory given as relative path.
Error observed in this case is:
"The PostgreSQL on Local Computer started and then stopped".
This may happen because relative path given will be relative to path from where service is registered but
the path from where WIN32 service execution gets invoked may be different and hence it won't be able
to find the data directory.
I have fixed the same by internally converting the relative path to absolute path as it is being done for executable file.

Hi,
I've not quite got around to testing this yet, but I think the patch is a good idea as I can see that I can use a relative path when I start postgres.exe from the command line, I guess the behaviour should likely be the same when installed as a windows
service.

Thanks for reviewing and providing the first level of feedback.

So, I've just been looking over this patch and I'm just wondering about a few things:

In pgwin32_CommandLine you declare dbPath, it looks like you could declare this in the scope of; if (pg_config)

Yes I have declared the same in the scope of "if (pg_config) "

In find_my_abs_path you're making a call to StrNCpy, I know you likely just used find_my_exec as an example here, but I'd say the use of StrNCpy is not really needed here and is a bit of a waste of cycles. I'd rather see strlcpy being used as strncpy
will needlessly zero the remaining buffer space.

Yes you are right, it is much better to use strlcpy instead of StrNCpy. I have modified the patch.

Also in find_my_abs_path, I'm just wondering if the cwd variable is really needed, I think you could just use retpath each time and it would also save a little bit of copying that's done in join_path_components(). By the looks of it you can just call
join_path_components(retpath, retpath, inpath). Perhaps some people would disagree, but I'm not really seeing the harm in it and it saves some copying too.

Yes I am also convinced with your suggestion. It really avoid one level of copy.
I have seen that the similar mechanism is used in many places where join_path_components() is called. So I think it should be OK to use in our case also.
I have made the necessary changes for the same.

I have attached the changed patch. Please provide your further feedback, if any.

Thanks and Regards,

Kumar Rajeev Rastogi

Attachments:

pgctl_win32service_rel_dbpath_v2.patchapplication/octet-stream; name=pgctl_win32service_rel_dbpath_v2.patchDownload

*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 1297,1303 **** pgwin32_CommandLine(bool registration)
  						  register_servicename);
  
  	if (pg_config)
! 		appendPQExpBuffer(cmdLine, " -D \"%s\"", pg_config);
  
  	if (registration && do_wait)
  		appendPQExpBuffer(cmdLine, " -w");
--- 1297,1314 ----
  						  register_servicename);
  
  	if (pg_config)
! 	{
! 		char		dbPath[MAXPGPATH];
! 
! 		if (find_my_abs_path(pg_config, dbPath) != 0)
! 		{
! 			write_stderr(_("%s: could not identify current directory\n"),
! 						 progname);
! 			exit(1);
! 		}
! 
! 		appendPQExpBuffer(cmdLine, " -D \"%s\"", dbPath);
! 	}
  
  	if (registration && do_wait)
  		appendPQExpBuffer(cmdLine, " -w");
*** a/src/common/exec.c
--- b/src/common/exec.c
***************
*** 586,591 **** set_pglocale_pgservice(const char *argv0, const char *app)
--- 586,626 ----
  	}
  }
  
+ /*
+  * find_my_abs_path -- find an absolute path to a given file/path
+  *
+  *	inpath is the input path for which absolute path required.
+  *	retpath is the output area (must be of size MAXPGPATH)
+  *	Returns 0 if OK, -1 if error.
+  */
+ int
+ find_my_abs_path(char *inpath, char *retpath)
+ {
+ 	/*
+ 	 * Check if it is already absolute path.
+ 	 */
+ 	if (is_absolute_path(inpath))
+ 	{
+ 		strlcpy(retpath, inpath, MAXPGPATH);
+ 	}
+ 	else
+ 	{
+ 		if (!getcwd(retpath, MAXPGPATH))
+ 		{
+ 			log_error(_("could not identify current directory: %s"),
+ 					  strerror(errno));
+ 			return -1;
+ 		}
+ 
+ 		join_path_components(retpath, retpath, inpath);
+ 		canonicalize_path(retpath);
+ 	}
+ 
+ 	/* use backslashes in path to avoid problems with some third-party tools */
+ 	make_native_path(retpath);
+ 	return resolve_symlinks(retpath);
+ }
+ 
  #ifdef WIN32
  
  /*
*** a/src/include/port.h
--- b/src/include/port.h
***************
*** 95,100 **** extern void set_pglocale_pgservice(const char *argv0, const char *app);
--- 95,101 ----
  extern int	find_my_exec(const char *argv0, char *retpath);
  extern int find_other_exec(const char *argv0, const char *target,
  				const char *versionstr, char *retpath);
+ extern int	find_my_abs_path(char *inpath, char *retpath);
  
  /* Windows security token manipulation (in exec.c) */
  #ifdef WIN32

Import Notes

Resolved by subject fallback