pgsql: pg_upgrade: quote directory names in delete_old_cluster script

Started by Bruce Momjianalmost 11 years ago9 messages
#1Bruce Momjian
bruce@momjian.us

pg_upgrade: quote directory names in delete_old_cluster script

This allows the delete script to properly function when special
characters appear in directory paths, e.g. spaces.

Backpatch through 9.0

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/056764b10248bff702d9d7b8b97690668eaf1c93

Modified Files
--------------
contrib/pg_upgrade/check.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

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

#2Peter Eisentraut
peter_e@gmx.net
In reply to: Bruce Momjian (#1)
Re: pgsql: pg_upgrade: quote directory names in delete_old_cluster script

On 2/11/15 10:06 PM, Bruce Momjian wrote:

pg_upgrade: quote directory names in delete_old_cluster script

This allows the delete script to properly function when special
characters appear in directory paths, e.g. spaces.

Wouldn't single quotes be better than double quotes if you want to
defend against special characters?

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

#3Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#2)
Re: pgsql: pg_upgrade: quote directory names in delete_old_cluster script

On Sun, Feb 15, 2015 at 12:02:44PM -0500, Peter Eisentraut wrote:

On 2/11/15 10:06 PM, Bruce Momjian wrote:

pg_upgrade: quote directory names in delete_old_cluster script

This allows the delete script to properly function when special
characters appear in directory paths, e.g. spaces.

Wouldn't single quotes be better than double quotes if you want to
defend against special characters?

I thought of that. Windows only does double-quotes, I think, and I
didn't see any value to doing a platform-specific quoting. Do you?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

#4Peter Eisentraut
peter_e@gmx.net
In reply to: Bruce Momjian (#3)
Re: pgsql: pg_upgrade: quote directory names in delete_old_cluster script

On 2/16/15 4:44 PM, Bruce Momjian wrote:

On Sun, Feb 15, 2015 at 12:02:44PM -0500, Peter Eisentraut wrote:

On 2/11/15 10:06 PM, Bruce Momjian wrote:

pg_upgrade: quote directory names in delete_old_cluster script

This allows the delete script to properly function when special
characters appear in directory paths, e.g. spaces.

Wouldn't single quotes be better than double quotes if you want to
defend against special characters?

I thought of that. Windows only does double-quotes, I think, and I
didn't see any value to doing a platform-specific quoting. Do you?

All of our makefiles use single quotes, so effectively the only
character we don't support in directory paths is the single quote itself.

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

#5Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#4)
Re: pgsql: pg_upgrade: quote directory names in delete_old_cluster script

On Mon, Feb 16, 2015 at 04:47:23PM -0500, Peter Eisentraut wrote:

On 2/16/15 4:44 PM, Bruce Momjian wrote:

On Sun, Feb 15, 2015 at 12:02:44PM -0500, Peter Eisentraut wrote:

On 2/11/15 10:06 PM, Bruce Momjian wrote:

pg_upgrade: quote directory names in delete_old_cluster script

This allows the delete script to properly function when special
characters appear in directory paths, e.g. spaces.

Wouldn't single quotes be better than double quotes if you want to
defend against special characters?

I thought of that. Windows only does double-quotes, I think, and I
didn't see any value to doing a platform-specific quoting. Do you?

All of our makefiles use single quotes, so effectively the only
character we don't support in directory paths is the single quote itself.

This seems to say that Windows batch files don't have any special
meaning for single quotes:

http://stackoverflow.com/questions/24173825/what-does-single-quote-do-in-windows-batch-files
http://stackoverflow.com/questions/10737283/single-quotes-and-double-quotes-how-to-have-the-same-behaviour-in-unix-and-wind

Again, is it worth doing something platform-specific? I can certainly
define a platform-specific macro for " or ' as and use that. Good idea?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

#6Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#5)
1 attachment(s)
Re: pg_upgrade: quote directory names in delete_old_cluster script

On Mon, Feb 16, 2015 at 05:03:45PM -0500, Bruce Momjian wrote:

All of our makefiles use single quotes, so effectively the only
character we don't support in directory paths is the single quote itself.

This seems to say that Windows batch files don't have any special
meaning for single quotes:

http://stackoverflow.com/questions/24173825/what-does-single-quote-do-in-windows-batch-files
http://stackoverflow.com/questions/10737283/single-quotes-and-double-quotes-how-to-have-the-same-behaviour-in-unix-and-wind

Again, is it worth doing something platform-specific? I can certainly
define a platform-specific macro for " or ' as and use that. Good idea?

I have developed the attached patch to use platform-specific quoting of
path names.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Attachments:

upgrade_quote.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 6db223a..be66b24
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** create_script_for_old_cluster_deletion(c
*** 532,538 ****
  #endif
  
  	/* delete old cluster's default tablespace */
! 	fprintf(script, RMDIR_CMD " \"%s\"\n", fix_path_separator(old_cluster.pgdata));
  
  	/* delete old cluster's alternate tablespaces */
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
--- 532,539 ----
  #endif
  
  	/* delete old cluster's default tablespace */
! 	fprintf(script, RMDIR_CMD " %c%s%c\n", PATH_QUOTE,
! 			fix_path_separator(old_cluster.pgdata), PATH_QUOTE);
  
  	/* delete old cluster's alternate tablespaces */
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
*************** create_script_for_old_cluster_deletion(c
*** 554,562 ****
  						PATH_SEPARATOR);
  
  			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
! 				fprintf(script, RMDIR_CMD " \"%s%c%d\"\n",
  						fix_path_separator(os_info.old_tablespaces[tblnum]),
! 						PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid);
  		}
  		else
  		{
--- 555,564 ----
  						PATH_SEPARATOR);
  
  			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
! 				fprintf(script, RMDIR_CMD " %c%s%c%d%c\n", PATH_QUOTE,
  						fix_path_separator(os_info.old_tablespaces[tblnum]),
! 						PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid,
! 						PATH_QUOTE);
  		}
  		else
  		{
*************** create_script_for_old_cluster_deletion(c
*** 566,574 ****
  			 * Simply delete the tablespace directory, which might be ".old"
  			 * or a version-specific subdirectory.
  			 */
! 			fprintf(script, RMDIR_CMD " \"%s%s\"\n",
  					fix_path_separator(os_info.old_tablespaces[tblnum]),
! 					fix_path_separator(suffix_path));
  			pfree(suffix_path);
  		}
  	}
--- 568,576 ----
  			 * Simply delete the tablespace directory, which might be ".old"
  			 * or a version-specific subdirectory.
  			 */
! 			fprintf(script, RMDIR_CMD " %c%s%s%c\n", PATH_QUOTE,
  					fix_path_separator(os_info.old_tablespaces[tblnum]),
! 					fix_path_separator(suffix_path), PATH_QUOTE);
  			pfree(suffix_path);
  		}
  	}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
new file mode 100644
index 4683c6f..bb035e1
*** a/src/bin/pg_upgrade/pg_upgrade.h
--- b/src/bin/pg_upgrade/pg_upgrade.h
*************** extern char *output_files[];
*** 74,79 ****
--- 74,80 ----
  #define pg_mv_file			rename
  #define pg_link_file		link
  #define PATH_SEPARATOR		'/'
+ #define PATH_QUOTE	'\''
  #define RM_CMD				"rm -f"
  #define RMDIR_CMD			"rm -rf"
  #define SCRIPT_PREFIX		"./"
*************** extern char *output_files[];
*** 85,90 ****
--- 86,92 ----
  #define pg_mv_file			pgrename
  #define pg_link_file		win32_pghardlink
  #define PATH_SEPARATOR		'\\'
+ #define PATH_QUOTE	'"'
  #define RM_CMD				"DEL /q"
  #define RMDIR_CMD			"RMDIR /s/q"
  #define SCRIPT_PREFIX		""
#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Bruce Momjian (#6)
Re: pg_upgrade: quote directory names in delete_old_cluster script

Bruce Momjian wrote:

I have developed the attached patch to use platform-specific quoting of
path names.

Part of me wonders about initdb's existing DIR_SEP and QUOTE_PATH
definitions ... seems messy to reinvent these things all over again.

--
�lvaro Herrera http://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

#8Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#7)
Re: pg_upgrade: quote directory names in delete_old_cluster script

On Wed, Apr 29, 2015 at 11:59:26PM -0300, Alvaro Herrera wrote:

Bruce Momjian wrote:

I have developed the attached patch to use platform-specific quoting of
path names.

Part of me wonders about initdb's existing DIR_SEP and QUOTE_PATH
definitions ... seems messy to reinvent these things all over again.

I don't think we can reuse QUOTE_PATH as it is used in initdb for
displaying potential ways of starting the server, and it assumes Unix
doesn't need quoting, while Windows usually does. For an actual script,
we always want to use quoting.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

#9Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#6)
Re: pg_upgrade: quote directory names in delete_old_cluster script

On Wed, Apr 29, 2015 at 10:52:45PM -0400, Bruce Momjian wrote:

On Mon, Feb 16, 2015 at 05:03:45PM -0500, Bruce Momjian wrote:

All of our makefiles use single quotes, so effectively the only
character we don't support in directory paths is the single quote itself.

This seems to say that Windows batch files don't have any special
meaning for single quotes:

http://stackoverflow.com/questions/24173825/what-does-single-quote-do-in-windows-batch-files
http://stackoverflow.com/questions/10737283/single-quotes-and-double-quotes-how-to-have-the-same-behaviour-in-unix-and-wind

Again, is it worth doing something platform-specific? I can certainly
define a platform-specific macro for " or ' as and use that. Good idea?

I have developed the attached patch to use platform-specific quoting of
path names.

Applied.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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