Re: [HACKERS] New pg_upgrade data directory inside old one?

[Bruce Momjian]

On Mon, Feb 15, 2016 at 06:32:06PM +0100, Magnus Hagander wrote:
> 
> 
> On Mon, Feb 15, 2016 at 6:29 PM, Bruce Momjian  wrote:
> 
> Someone on IRC reported that if they had run the pg_upgrade-created
> delete_old_cluster.sh shell script it would have deleted their old _and_
> new data directories.  (Fortunately they didn't run it.)
> 
> I was confused how this could have happened, and the user explained that
> their old cluster was in /u/pgsql/data, and that they wanted to switch to
> a per-major-version directory naming schema, so they put the new data
> directory in /u/pgsql/data/9.5.  (They could have just moved the
> directory while the server was down, but didn't.)
> 
> Unfortunately, there is no check for having the new cluster data
> directory inside the old data directory, only a check for tablespace
> directories in the old cluster.  (I never anticipated someone would do
> this.)
> 
> 
> Interesting - I definitely wouldn't have expected that either. And it
> definitely seems like a foot-gun we should protect the users against. 

Patch applied back through 9.3 where delete script tests were added.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
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] New pg_upgrade data directory inside old one?

[Magnus Hagander]

On Mon, Feb 15, 2016 at 6:29 PM, Bruce Momjian  wrote:

> Someone on IRC reported that if they had run the pg_upgrade-created
> delete_old_cluster.sh shell script it would have deleted their old _and_
> new data directories.  (Fortunately they didn't run it.)
>
> I was confused how this could have happened, and the user explained that
> their old cluster was in /u/pgsql/data, and that they wanted to switch to
> a per-major-version directory naming schema, so they put the new data
> directory in /u/pgsql/data/9.5.  (They could have just moved the
> directory while the server was down, but didn't.)
>
> Unfortunately, there is no check for having the new cluster data
> directory inside the old data directory, only a check for tablespace
> directories in the old cluster.  (I never anticipated someone would do
> this.)
>

Interesting - I definitely wouldn't have expected that either. And it
definitely seems like a foot-gun we should protect the users against.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

[HACKERS] New pg_upgrade data directory inside old one?

[Bruce Momjian]

Someone on IRC reported that if they had run the pg_upgrade-created
delete_old_cluster.sh shell script it would have deleted their old _and_
new data directories.  (Fortunately they didn't run it.)

I was confused how this could have happened, and the user explained that
their old cluster was in /u/pgsql/data, and that they wanted to switch to
a per-major-version directory naming schema, so they put the new data
directory in /u/pgsql/data/9.5.  (They could have just moved the
directory while the server was down, but didn't.)

Unfortunately, there is no check for having the new cluster data
directory inside the old data directory, only a check for tablespace
directories in the old cluster.  (I never anticipated someone would do
this.)

The attached patch adds the proper check.  This should be backpatched to
all supported Postgres versions.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 8c034bc..e92fc66
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*** output_completion_banner(char *analyze_s
*** 195,203 
  			   deletion_script_file_name);
  	else
  		pg_log(PG_REPORT,
! 			   "Could not create a script to delete the old cluster's data\n"
! 		  "files because user-defined tablespaces exist in the old cluster\n"
! 		"directory.  The old cluster's contents must be deleted manually.\n");
  }
  
  
--- 195,204 
  			   deletion_script_file_name);
  	else
  		pg_log(PG_REPORT,
! 		  "Could not create a script to delete the old cluster's data files\n"
! 		  "because the new cluster's data directory or user-defined tablespaces\n"
! 		  "exist in the old cluster directory.  The old cluster's contents must\n"
! 		  "be deleted manually.\n");
  }
  
  
*** create_script_for_old_cluster_deletion(c
*** 496,513 
  {
  	FILE	   *script = NULL;
  	int			tblnum;
! 	char		old_cluster_pgdata[MAXPGPATH];
  
  	*deletion_script_file_name = psprintf("%sdelete_old_cluster.%s",
  		  SCRIPT_PREFIX, SCRIPT_EXT);
  
  	/*
  	 * Some users (oddly) create tablespaces inside the cluster data
  	 * directory.  We can't create a proper old cluster delete script in that
  	 * case.
  	 */
- 	strlcpy(old_cluster_pgdata, old_cluster.pgdata, MAXPGPATH);
- 	canonicalize_path(old_cluster_pgdata);
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
  	{
  		char		old_tablespace_dir[MAXPGPATH];
--- 497,531 
  {
  	FILE	   *script = NULL;
  	int			tblnum;
! 	char		old_cluster_pgdata[MAXPGPATH], new_cluster_pgdata[MAXPGPATH];
  
  	*deletion_script_file_name = psprintf("%sdelete_old_cluster.%s",
  		  SCRIPT_PREFIX, SCRIPT_EXT);
  
+ 	strlcpy(old_cluster_pgdata, old_cluster.pgdata, MAXPGPATH);
+ 	canonicalize_path(old_cluster_pgdata);
+ 
+ 	strlcpy(new_cluster_pgdata, new_cluster.pgdata, MAXPGPATH);
+ 	canonicalize_path(new_cluster_pgdata);
+ 
+ 	/* Some people put the new data directory inside the old one. */
+ 	if (path_is_prefix_of_path(old_cluster_pgdata, new_cluster_pgdata))
+ 	{
+ 		pg_log(PG_WARNING,
+ 		   "\nWARNING:  new data directory should not be inside the old data directory, e.g. %s\n", old_cluster_pgdata);
+ 
+ 		/* Unlink file in case it is left over from a previous run. */
+ 		unlink(*deletion_script_file_name);
+ 		pg_free(*deletion_script_file_name);
+ 		*deletion_script_file_name = NULL;
+ 		return;
+ 	}
+ 
  	/*
  	 * Some users (oddly) create tablespaces inside the cluster data
  	 * directory.  We can't create a proper old cluster delete script in that
  	 * case.
  	 */
  	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
  	{
  		char		old_tablespace_dir[MAXPGPATH];

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