Обсуждение: Re: create tablespace fails silently, or succeeds improperly

bruce wrote:
> Tom Lane wrote:
> > Dave Cramer <pg@fastcrypt.com> writes:
> > > as seen below create tablespace does not throw an error or appear to
> > > do anything other than register the tablespace.
> >
> > I suspect this behavior is partially intentional, because tablespace
> > creation now involves an extra level of subdirectory.  However, it's
> > not clear to me why CREATE TABLESPACE is still changing the permissions
> > on the parent directory.  Bruce, exactly what is the rationale here?
>
> Well, the symbolic link from data/pg_tblspc points to the top directory,
> not to the catalog-version-named subdirectory.  This was done for
> several reasons, particularly so the directory pointed to by the symlink
> would be exactly the same as that specified by CREATE TABLESPACE, for
> code clarity.

Looking at the pg_upgrade code some more, I found that it was not
removing the PG_VERSION file when deleting <= 8.4 tablespace files.
This might confuse administrators so the attached patch adds the removal
of PG_VERSION.  I would like to apply this to master and 9.0.X.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 144fcdc..0492657 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -416,6 +416,11 @@ create_script_for_old_cluster_deletion(migratorContext *ctx,
             int            dbnum;

             fprintf(script, "\n");
+            /* remove PG_VERSION? */
+            if (GET_MAJOR_VERSION(ctx->old.major_version) <= 804)
+                fprintf(script, RM_CMD " %s%s/PG_VERSION\n",
+                        ctx->tablespaces[tblnum], ctx->old.tablespace_suffix);
+
             for (dbnum = 0; dbnum < ctx->new.dbarr.ndbs; dbnum++)
             {
                 fprintf(script, RMDIR_CMD " %s%s/%d\n",
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index 73070c6..52d4a82 100644
--- a/contrib/pg_upgrade/pg_upgrade.h
+++ b/contrib/pg_upgrade/pg_upgrade.h
@@ -38,6 +38,7 @@
 #define pg_copy_file        copy_file
 #define pg_mv_file            rename
 #define pg_link_file        link
+#define RM_CMD                "rm -f"
 #define RMDIR_CMD            "rm -rf"
 #define EXEC_EXT            "sh"
 #else
@@ -45,6 +46,7 @@
 #define pg_mv_file            pgrename
 #define pg_link_file        win32_pghardlink
 #define sleep(x)            Sleep(x * 1000)
+#define RM_CMD                "DEL /q"
 #define RMDIR_CMD            "RMDIR /s/q"
 #define EXEC_EXT            "bat"
 #define EXE_EXT                ".exe"

Bruce Momjian <bruce@momjian.us> writes:
> Looking at the pg_upgrade code some more, I found that it was not
> removing the PG_VERSION file when deleting <= 8.4 tablespace files. 
> This might confuse administrators so the attached patch adds the removal
> of PG_VERSION.  I would like to apply this to master and 9.0.X.

... why is that a good idea?
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Looking at the pg_upgrade code some more, I found that it was not
> > removing the PG_VERSION file when deleting <= 8.4 tablespace files. 
> > This might confuse administrators so the attached patch adds the removal
> > of PG_VERSION.  I would like to apply this to master and 9.0.X.
> 
> ... why is that a good idea?

The script already deletes the 8.4 database directories, but leaves
PG_VERSION behind.  Why keep it when all the 8.4 data is gone?  The
script also dates PGDATA for 8.4, so there is nothing left pointing to
that directory.  Here are the delete script file contents after the
patch:
#!/bin/shrm -rf /u/pgsql.old/datarm -f /rtmp/pgsql/PG_VERSIONrm -rf /rtmp/pgsql/1rm -rf /rtmp/pgsql/11564rm -rf
/rtmp/pgsql/16384rm-rf /rtmp/pgsql/16385rm -rf /rtmp/pgsql/27628
 


--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> Bruce Momjian <bruce@momjian.us> writes:
>>> Looking at the pg_upgrade code some more, I found that it was not
>>> removing the PG_VERSION file when deleting <= 8.4 tablespace files. 
>>> This might confuse administrators so the attached patch adds the removal
>>> of PG_VERSION.  I would like to apply this to master and 9.0.X.
>> 
>> ... why is that a good idea?

> The script already deletes the 8.4 database directories, but leaves
> PG_VERSION behind.  Why keep it when all the 8.4 data is gone?  The
> script also dates PGDATA for 8.4, so there is nothing left pointing to
> that directory.

Oh, I misunderstood: I thought you were proposing to do this as an
automatic action inside pg_upgrade.  If it's part of the cleanup script,
it's fine.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> Bruce Momjian <bruce@momjian.us> writes:
> >>> Looking at the pg_upgrade code some more, I found that it was not
> >>> removing the PG_VERSION file when deleting <= 8.4 tablespace files. 
> >>> This might confuse administrators so the attached patch adds the removal
> >>> of PG_VERSION.  I would like to apply this to master and 9.0.X.
> >> 
> >> ... why is that a good idea?
> 
> > The script already deletes the 8.4 database directories, but leaves
> > PG_VERSION behind.  Why keep it when all the 8.4 data is gone?  The
> > script also dates PGDATA for 8.4, so there is nothing left pointing to
> > that directory.
> 
> Oh, I misunderstood: I thought you were proposing to do this as an
> automatic action inside pg_upgrade.  If it's part of the cleanup script,
> it's fine.

Yes, never automatic.  You can always roll back after pg_upgrade
completes.  Once you start the new server, only then can you not go
back.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +