Re: [HACKERS] Fix for pg_upgrade and invalid indexes

2013-03-30 Thread Andres Freund
On 2013-03-29 19:03:05 -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
  clumsy.
 
 That was what I started to write, too, but actually I think the IS
 DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
 that the query appears to be intended to collect regular tables as
 well as indexes.  (As patched, that's totally broken, so I infer
 Bruce hasn't tested it yet.)

Ah yes. Then I'd actually find it much more readable to formulate it as a NOT
EXISTS(), but that might be just me.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Fix for pg_upgrade and invalid indexes

2013-03-30 Thread Bruce Momjian

OK, patch applied and backpatched as far back as pg_upgrade exists in
git.

---

On Fri, Mar 29, 2013 at 11:35:13PM -0400, Bruce Momjian wrote:
 On Fri, Mar 29, 2013 at 07:03:05PM -0400, Tom Lane wrote:
  Andres Freund and...@2ndquadrant.com writes:
   Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
   clumsy.
  
  That was what I started to write, too, but actually I think the IS
  DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
  that the query appears to be intended to collect regular tables as
  well as indexes.  (As patched, that's totally broken, so I infer
  Bruce hasn't tested it yet.)
 
 Yes, I only ran my simple tests so far --- I wanted to at least get some
 eyes on it.  I was wondering if we ever need to use parentheses for
 queries that mix normal and outer joins?  I am unclear on that.
 
 Attached is a fixed patch that uses LEFT JOIN.  I went back and looked
 at the patch that added this test and I think the patch is now complete.
 I would like to apply it tomorrow/Saturday so it will be ready for
 Monday's packaging, and get some buildfarm time on it.
 
 -- 
   Bruce Momjian  br...@momjian.ushttp://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
 new file mode 100644
 index 65fb548..35783d0
 *** a/contrib/pg_upgrade/check.c
 --- b/contrib/pg_upgrade/check.c
 *** static void check_is_super_user(ClusterI
 *** 20,26 
   static void check_for_prepared_transactions(ClusterInfo *cluster);
   static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
   static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 - static void check_for_invalid_indexes(ClusterInfo *cluster);
   static void get_bin_version(ClusterInfo *cluster);
   static char *get_canonical_locale_name(int category, const char *locale);
   
 --- 20,25 
 *** check_and_dump_old_cluster(bool live_che
 *** 97,103 
   check_is_super_user(old_cluster);
   check_for_prepared_transactions(old_cluster);
   check_for_reg_data_type_usage(old_cluster);
 - check_for_invalid_indexes(old_cluster);
   check_for_isn_and_int8_passing_mismatch(old_cluster);
   
   /* old = PG 8.3 checks? */
 --- 96,101 
 *** check_for_reg_data_type_usage(ClusterInf
 *** 952,1046 
  %s\n\n, output_path);
   }
   else
 - check_ok();
 - }
 - 
 - 
 - /*
 -  * check_for_invalid_indexes()
 -  *
 -  *  CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
 -  *  fails.  These are dumped as valid indexes by pg_dump, but the
 -  *  underlying files are still invalid indexes.  This checks to make sure
 -  *  no invalid indexes exist, either failed index builds or concurrent
 -  *  indexes in the process of being created.
 -  */
 - static void
 - check_for_invalid_indexes(ClusterInfo *cluster)
 - {
 - int dbnum;
 - FILE   *script = NULL;
 - boolfound = false;
 - charoutput_path[MAXPGPATH];
 - 
 - prep_status(Checking for invalid indexes from concurrent index 
 builds);
 - 
 - snprintf(output_path, sizeof(output_path), invalid_indexes.txt);
 - 
 - for (dbnum = 0; dbnum  cluster-dbarr.ndbs; dbnum++)
 - {
 - PGresult   *res;
 - booldb_used = false;
 - int ntups;
 - int rowno;
 - int i_nspname,
 - i_relname;
 - DbInfo *active_db = cluster-dbarr.dbs[dbnum];
 - PGconn *conn = connectToServer(cluster, active_db-db_name);
 - 
 - res = executeQueryOrDie(conn,
 - SELECT 
 n.nspname, c.relname 
 - FROM   
 pg_catalog.pg_class c, 
 -
 pg_catalog.pg_namespace n, 
 -
 pg_catalog.pg_index i 
 - WHERE  
 (i.indisvalid = false OR 
 -
  i.indisready = false) AND 
 -
 i.indexrelid = c.oid AND 
 -
 c.relnamespace = n.oid AND 
 - /* we do not 
 migrate these, so skip them */
 -
 n.nspname != 

[HACKERS] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Bruce Momjian
On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Should I just patch pg_upgrade to remove the indisvalid, skip
  indisvalid indexes, and backpatch it?  Users should be using the
  version of pg_upgrade to match new pg_dump.  Is there any case where
  they don't match?  Do I still need to check for indisready?
 
 Yeah, if you can just ignore !indisvalid indexes that should work fine.
 I see no need to look at indisready if you're doing that.

Attached is a patch that implements the suggested pg_upgrade changes of
not copying invalid indexes now that pg_dump doesn't dump them.  This
should be backpatched back to 8.4 to match pg_dump.  It might require
release note updates;  not sure.  Previously pg_upgrade threw an error
if invalid indexes exist, but only since February, when we released the
pg_upgrade fix to do this.  You can see the majority of this patch is
removing that check.

-- 
  Bruce Momjian  br...@momjian.ushttp://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
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26 
  static void check_for_prepared_transactions(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
  static void get_bin_version(ClusterInfo *cluster);
  static char *get_canonical_locale_name(int category, const char *locale);
  
--- 20,25 
*** check_and_dump_old_cluster(bool live_che
*** 97,103 
  	check_is_super_user(old_cluster);
  	check_for_prepared_transactions(old_cluster);
  	check_for_reg_data_type_usage(old_cluster);
- 	check_for_invalid_indexes(old_cluster);
  	check_for_isn_and_int8_passing_mismatch(old_cluster);
  
  	/* old = PG 8.3 checks? */
--- 96,101 
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046 
  			   %s\n\n, output_path);
  	}
  	else
- 		check_ok();
- }
- 
- 
- /*
-  * check_for_invalid_indexes()
-  *
-  *	CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
-  *	fails.  These are dumped as valid indexes by pg_dump, but the
-  *	underlying files are still invalid indexes.  This checks to make sure
-  *	no invalid indexes exist, either failed index builds or concurrent
-  *	indexes in the process of being created.
-  */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- 	int			dbnum;
- 	FILE	   *script = NULL;
- 	bool		found = false;
- 	char		output_path[MAXPGPATH];
- 
- 	prep_status(Checking for invalid indexes from concurrent index builds);
- 
- 	snprintf(output_path, sizeof(output_path), invalid_indexes.txt);
- 
- 	for (dbnum = 0; dbnum  cluster-dbarr.ndbs; dbnum++)
- 	{
- 		PGresult   *res;
- 		bool		db_used = false;
- 		int			ntups;
- 		int			rowno;
- 		int			i_nspname,
- 	i_relname;
- 		DbInfo	   *active_db = cluster-dbarr.dbs[dbnum];
- 		PGconn	   *conn = connectToServer(cluster, active_db-db_name);
- 
- 		res = executeQueryOrDie(conn,
- SELECT n.nspname, c.relname 
- FROM	pg_catalog.pg_class c, 
- 		pg_catalog.pg_namespace n, 
- 		pg_catalog.pg_index i 
- WHERE	(i.indisvalid = false OR 
- 		 i.indisready = false) AND 
- 		i.indexrelid = c.oid AND 
- 		c.relnamespace = n.oid AND 
- /* we do not migrate these, so skip them */
- 			 		n.nspname != 'pg_catalog' AND 
- 		n.nspname != 'information_schema' AND 
- /* indexes do not have toast tables */
- 		n.nspname != 'pg_toast');
- 
- 		ntups = PQntuples(res);
- 		i_nspname = PQfnumber(res, nspname);
- 		i_relname = PQfnumber(res, relname);
- 		for (rowno = 0; rowno  ntups; rowno++)
- 		{
- 			found = true;
- 			if (script == NULL  (script = fopen_priv(output_path, w)) == NULL)
- pg_log(PG_FATAL, Could not open file \%s\: %s\n,
- 	   output_path, getErrorText(errno));
- 			if (!db_used)
- 			{
- fprintf(script, Database: %s\n, active_db-db_name);
- db_used = true;
- 			}
- 			fprintf(script,   %s.%s\n,
- 	PQgetvalue(res, rowno, i_nspname),
- 	PQgetvalue(res, rowno, i_relname));
- 		}
- 
- 		PQclear(res);
- 
- 		PQfinish(conn);
- 	}
- 
- 	if (script)
- 		fclose(script);
- 
- 	if (found)
- 	{
- 		pg_log(PG_REPORT, fatal\n);
- 		pg_log(PG_FATAL,
- 			   Your installation contains invalid indexes due to failed or\n
- 		 	   currently running CREATE INDEX CONCURRENTLY operations.  You\n
- 			   cannot upgrade until these indexes are valid or removed.  A\n
- 			   list of the problem indexes is in the file:\n
- 			   %s\n\n, output_path);
- 	}
- 	else
  		check_ok();
  }
  
--- 950,955 

Re: [HACKERS] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Attached is a patch that implements the suggested pg_upgrade changes of
 not copying invalid indexes now that pg_dump doesn't dump them.  This
 should be backpatched back to 8.4 to match pg_dump.  It might require
 release note updates;  not sure.  Previously pg_upgrade threw an error
 if invalid indexes exist, but only since February, when we released the
 pg_upgrade fix to do this.  You can see the majority of this patch is
 removing that check.

Surely that should be LEFT JOIN not RIGHT JOIN?

regards, tom lane


-- 
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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Andres Freund
On 2013-03-29 16:57:06 -0400, Bruce Momjian wrote:
 On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote:
  Bruce Momjian br...@momjian.us writes:
   Should I just patch pg_upgrade to remove the indisvalid, skip
   indisvalid indexes, and backpatch it?  Users should be using the
   version of pg_upgrade to match new pg_dump.  Is there any case where
   they don't match?  Do I still need to check for indisready?
  
  Yeah, if you can just ignore !indisvalid indexes that should work fine.
  I see no need to look at indisready if you're doing that.
 
 Attached is a patch that implements the suggested pg_upgrade changes of
 not copying invalid indexes now that pg_dump doesn't dump them.  This
 should be backpatched back to 8.4 to match pg_dump.  It might require
 release note updates;  not sure.  Previously pg_upgrade threw an error
 if invalid indexes exist, but only since February, when we released the
 pg_upgrade fix to do this.  You can see the majority of this patch is
 removing that check.

 +  RIGHT OUTER JOIN pg_catalog.pg_index i 
 +   ON (c.oid = i.indexrelid) 
WHERE relkind IN ('r', 'm', 'i'%s) AND 
 + /* pg_dump only dumps valid indexes;  testing 
 indisready is
 +  * necessary in 9.2, and harmless in earlier/later 
 versions. */
 +   i.indisvalid IS DISTINCT FROM false AND 
 +   i.indisready IS DISTINCT FROM false AND 
   /* exclude possible orphaned temp tables */
  ((n.nspname !~ '^pg_temp_' AND 
n.nspname !~ '^pg_toast_temp_' AND 

Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
clumsy.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
 clumsy.

That was what I started to write, too, but actually I think the IS
DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
that the query appears to be intended to collect regular tables as
well as indexes.  (As patched, that's totally broken, so I infer
Bruce hasn't tested it yet.)

regards, tom lane


-- 
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] Fix for pg_upgrade and invalid indexes

2013-03-29 Thread Bruce Momjian
On Fri, Mar 29, 2013 at 07:03:05PM -0400, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  Those columns cannot be NULL, so using IS DISTINCT FROM seems a bit
  clumsy.
 
 That was what I started to write, too, but actually I think the IS
 DISTINCT is correct and the RIGHT JOIN should be a LEFT JOIN.  Note
 that the query appears to be intended to collect regular tables as
 well as indexes.  (As patched, that's totally broken, so I infer
 Bruce hasn't tested it yet.)

Yes, I only ran my simple tests so far --- I wanted to at least get some
eyes on it.  I was wondering if we ever need to use parentheses for
queries that mix normal and outer joins?  I am unclear on that.

Attached is a fixed patch that uses LEFT JOIN.  I went back and looked
at the patch that added this test and I think the patch is now complete.
I would like to apply it tomorrow/Saturday so it will be ready for
Monday's packaging, and get some buildfarm time on it.

-- 
  Bruce Momjian  br...@momjian.ushttp://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
new file mode 100644
index 65fb548..35783d0
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_is_super_user(ClusterI
*** 20,26 
  static void check_for_prepared_transactions(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
- static void check_for_invalid_indexes(ClusterInfo *cluster);
  static void get_bin_version(ClusterInfo *cluster);
  static char *get_canonical_locale_name(int category, const char *locale);
  
--- 20,25 
*** check_and_dump_old_cluster(bool live_che
*** 97,103 
  	check_is_super_user(old_cluster);
  	check_for_prepared_transactions(old_cluster);
  	check_for_reg_data_type_usage(old_cluster);
- 	check_for_invalid_indexes(old_cluster);
  	check_for_isn_and_int8_passing_mismatch(old_cluster);
  
  	/* old = PG 8.3 checks? */
--- 96,101 
*** check_for_reg_data_type_usage(ClusterInf
*** 952,1046 
  			   %s\n\n, output_path);
  	}
  	else
- 		check_ok();
- }
- 
- 
- /*
-  * check_for_invalid_indexes()
-  *
-  *	CREATE INDEX CONCURRENTLY can create invalid indexes if the index build
-  *	fails.  These are dumped as valid indexes by pg_dump, but the
-  *	underlying files are still invalid indexes.  This checks to make sure
-  *	no invalid indexes exist, either failed index builds or concurrent
-  *	indexes in the process of being created.
-  */
- static void
- check_for_invalid_indexes(ClusterInfo *cluster)
- {
- 	int			dbnum;
- 	FILE	   *script = NULL;
- 	bool		found = false;
- 	char		output_path[MAXPGPATH];
- 
- 	prep_status(Checking for invalid indexes from concurrent index builds);
- 
- 	snprintf(output_path, sizeof(output_path), invalid_indexes.txt);
- 
- 	for (dbnum = 0; dbnum  cluster-dbarr.ndbs; dbnum++)
- 	{
- 		PGresult   *res;
- 		bool		db_used = false;
- 		int			ntups;
- 		int			rowno;
- 		int			i_nspname,
- 	i_relname;
- 		DbInfo	   *active_db = cluster-dbarr.dbs[dbnum];
- 		PGconn	   *conn = connectToServer(cluster, active_db-db_name);
- 
- 		res = executeQueryOrDie(conn,
- SELECT n.nspname, c.relname 
- FROM	pg_catalog.pg_class c, 
- 		pg_catalog.pg_namespace n, 
- 		pg_catalog.pg_index i 
- WHERE	(i.indisvalid = false OR 
- 		 i.indisready = false) AND 
- 		i.indexrelid = c.oid AND 
- 		c.relnamespace = n.oid AND 
- /* we do not migrate these, so skip them */
- 			 		n.nspname != 'pg_catalog' AND 
- 		n.nspname != 'information_schema' AND 
- /* indexes do not have toast tables */
- 		n.nspname != 'pg_toast');
- 
- 		ntups = PQntuples(res);
- 		i_nspname = PQfnumber(res, nspname);
- 		i_relname = PQfnumber(res, relname);
- 		for (rowno = 0; rowno  ntups; rowno++)
- 		{
- 			found = true;
- 			if (script == NULL  (script = fopen_priv(output_path, w)) == NULL)
- pg_log(PG_FATAL, Could not open file \%s\: %s\n,
- 	   output_path, getErrorText(errno));
- 			if (!db_used)
- 			{
- fprintf(script, Database: %s\n, active_db-db_name);
- db_used = true;
- 			}
- 			fprintf(script,   %s.%s\n,
- 	PQgetvalue(res, rowno, i_nspname),
- 	PQgetvalue(res, rowno, i_relname));
- 		}
- 
- 		PQclear(res);
- 
- 		PQfinish(conn);
- 	}
- 
- 	if (script)
- 		fclose(script);
- 
- 	if (found)
- 	{
- 		pg_log(PG_REPORT, fatal\n);
- 		pg_log(PG_FATAL,
- 			   Your installation contains invalid indexes due to failed or\n
- 		 	   currently running CREATE INDEX CONCURRENTLY operations.  You\n
- 			   cannot upgrade until these indexes are valid or removed.  A\n
- 			   list of the problem indexes is in the file:\n
- 			   %s\n\n, output_path);
- 	}
- 	else
  		check_ok();
  }
 

Re: [HACKERS] Fix for pg_upgrade status display

2012-12-07 Thread Bruce Momjian
On Wed, Dec  5, 2012 at 10:04:53PM -0500, Bruce Momjian wrote:
 Pg_upgrade displays file names during copy and database names during
 dump/restore.  Andrew Dunstan identified three bugs:
 
 *  long file names were being truncated to 60 _leading_ characters, which
 often do not change for long file names
 
 *  file names were truncated to 60 characters in log files
 
 *  carriage returns were being output to log files
 
 The attached patch fixes these --- it prints 60 _trailing_ characters to
 the status display, and full path names without carriage returns to log
 files.

Patch applied.   It also suppresses status output to the log file unless
verbose mode is used.

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

  + It's impossible for everything to be true. +


-- 
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] Fix for pg_upgrade status display

2012-12-06 Thread Robert Haas
On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian br...@momjian.us wrote:
 Pg_upgrade displays file names during copy and database names during
 dump/restore.  Andrew Dunstan identified three bugs:

 *  long file names were being truncated to 60 _leading_ characters, which
 often do not change for long file names

 *  file names were truncated to 60 characters in log files

 *  carriage returns were being output to log files

 The attached patch fixes these --- it prints 60 _trailing_ characters to
 the status display, and full path names without carriage returns to log
 files.

This might be a dumb question, but why limit it to 60 characters at
all instead of, say, MAXPGPATH?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Fix for pg_upgrade status display

2012-12-06 Thread Alvaro Herrera
Robert Haas escribió:
 On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian br...@momjian.us wrote:
  Pg_upgrade displays file names during copy and database names during
  dump/restore.  Andrew Dunstan identified three bugs:
 
  *  long file names were being truncated to 60 _leading_ characters, which
  often do not change for long file names
 
  *  file names were truncated to 60 characters in log files
 
  *  carriage returns were being output to log files
 
  The attached patch fixes these --- it prints 60 _trailing_ characters to
  the status display, and full path names without carriage returns to log
  files.
 
 This might be a dumb question, but why limit it to 60 characters at
 all instead of, say, MAXPGPATH?

I think this should be keyed off the terminal width, actually, no?  The
whole point of this is to overwrite the same line over and over, right?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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] Fix for pg_upgrade status display

2012-12-06 Thread Bruce Momjian

On Thu, Dec  6, 2012 at 12:43:53PM -0500, Robert Haas wrote:
 On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian br...@momjian.us wrote:
  Pg_upgrade displays file names during copy and database names during
  dump/restore.  Andrew Dunstan identified three bugs:
 
  *  long file names were being truncated to 60 _leading_ characters, which
  often do not change for long file names
 
  *  file names were truncated to 60 characters in log files
 
  *  carriage returns were being output to log files
 
  The attached patch fixes these --- it prints 60 _trailing_ characters to
  the status display, and full path names without carriage returns to log
  files.
 
 This might be a dumb question, but why limit it to 60 characters at
 all instead of, say, MAXPGPATH?

It is limited to 60 only for screen display, so the user knows what is
being processed.  If the text wraps across several lines, the \r trick
to overwrite the string will not work.

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

  + It's impossible for everything to be true. +


-- 
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] Fix for pg_upgrade status display

2012-12-06 Thread Bruce Momjian
On Thu, Dec  6, 2012 at 02:53:44PM -0300, Alvaro Herrera wrote:
 Robert Haas escribió:
  On Wed, Dec 5, 2012 at 10:04 PM, Bruce Momjian br...@momjian.us wrote:
   Pg_upgrade displays file names during copy and database names during
   dump/restore.  Andrew Dunstan identified three bugs:
  
   *  long file names were being truncated to 60 _leading_ characters, which
   often do not change for long file names
  
   *  file names were truncated to 60 characters in log files
  
   *  carriage returns were being output to log files
  
   The attached patch fixes these --- it prints 60 _trailing_ characters to
   the status display, and full path names without carriage returns to log
   files.
  
  This might be a dumb question, but why limit it to 60 characters at
  all instead of, say, MAXPGPATH?
 
 I think this should be keyed off the terminal width, actually, no?  The
 whole point of this is to overwrite the same line over and over, right?

That seems like overkill for a status message.  It is just there so
users know pg_upgrade isn't stuck, which was the complaint before the
message was used.

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

  + It's impossible for everything to be true. +


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


[HACKERS] Fix for pg_upgrade status display

2012-12-05 Thread Bruce Momjian
Pg_upgrade displays file names during copy and database names during
dump/restore.  Andrew Dunstan identified three bugs:

*  long file names were being truncated to 60 _leading_ characters, which
often do not change for long file names

*  file names were truncated to 60 characters in log files

*  carriage returns were being output to log files

The attached patch fixes these --- it prints 60 _trailing_ characters to
the status display, and full path names without carriage returns to log
files.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
new file mode 100644
index 2c1b65b..f35852b
*** a/contrib/pg_upgrade/dump.c
--- b/contrib/pg_upgrade/dump.c
*** generate_old_dump(void)
*** 36,42 
  		char 		file_name[MAXPGPATH];
  		DbInfo *old_db = old_cluster.dbarr.dbs[dbnum];
  
! 		pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_db-db_name);
  		snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db-db_oid);
  
  		exec_prog(RESTORE_LOG_FILE, NULL, true,
--- 36,42 
  		char 		file_name[MAXPGPATH];
  		DbInfo *old_db = old_cluster.dbarr.dbs[dbnum];
  
! 		pg_log(PG_STATUS, %s, old_db-db_name);
  		snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db-db_oid);
  
  		exec_prog(RESTORE_LOG_FILE, NULL, true,
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
new file mode 100644
index 63df529..2d4b678
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
*** create_new_objects(void)
*** 310,316 
  		char file_name[MAXPGPATH];
  		DbInfo *old_db = old_cluster.dbarr.dbs[dbnum];
  
! 		pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_db-db_name);
  		snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db-db_oid);
  
  		/*
--- 310,316 
  		char file_name[MAXPGPATH];
  		DbInfo *old_db = old_cluster.dbarr.dbs[dbnum];
  
! 		pg_log(PG_STATUS, %s, old_db-db_name);
  		snprintf(file_name, sizeof(file_name), DB_DUMP_FILE_MASK, old_db-db_oid);
  
  		/*
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index d981035..972e8e9
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
***
*** 24,32 
  
  #define MIGRATOR_API_VERSION	1
  
! #define MESSAGE_WIDTH		60
  
- #define OVERWRITE_MESSAGE	  %- MESSAGE_WIDTH . MESSAGE_WIDTH s\r
  #define GET_MAJOR_VERSION(v)	((v) / 100)
  
  /* contains both global db information and CREATE DATABASE commands */
--- 24,31 
  
  #define MIGRATOR_API_VERSION	1
  
! #define MESSAGE_WIDTH		60
  
  #define GET_MAJOR_VERSION(v)	((v) / 100)
  
  /* contains both global db information and CREATE DATABASE commands */
*** typedef enum
*** 208,213 
--- 207,213 
  typedef enum
  {
  	PG_VERBOSE,
+ 	PG_STATUS,
  	PG_REPORT,
  	PG_WARNING,
  	PG_FATAL
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 14e66df..5fec5ad
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*** transfer_relfile(pageCnvCtx *pageConvert
*** 213,219 
  		unlink(new_file);
  	
  		/* Copying files might take some time, so give feedback. */
! 		pg_log(PG_REPORT, OVERWRITE_MESSAGE, old_file);
  	
  		if ((user_opts.transfer_mode == TRANSFER_MODE_LINK)  (pageConverter != NULL))
  			pg_log(PG_FATAL, This upgrade requires page-by-page conversion, 
--- 213,219 
  		unlink(new_file);
  	
  		/* Copying files might take some time, so give feedback. */
! 		pg_log(PG_STATUS, %s, old_file);
  	
  		if ((user_opts.transfer_mode == TRANSFER_MODE_LINK)  (pageConverter != NULL))
  			pg_log(PG_FATAL, This upgrade requires page-by-page conversion, 
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
new file mode 100644
index 0c1eccc..e3fdf3a
*** a/contrib/pg_upgrade/util.c
--- b/contrib/pg_upgrade/util.c
*** prep_status(const char *fmt,...)
*** 75,81 
  	if (strlen(message)  0  message[strlen(message) - 1] == '\n')
  		pg_log(PG_REPORT, %s, message);
  	else
! 		pg_log(PG_REPORT, %- MESSAGE_WIDTH s, message);
  }
  
  
--- 75,82 
  	if (strlen(message)  0  message[strlen(message) - 1] == '\n')
  		pg_log(PG_REPORT, %s, message);
  	else
! 		/* trim strings that don't end in a newline */
! 		pg_log(PG_REPORT, %-*s, MESSAGE_WIDTH, message);
  }
  
  
*** pg_log(eLogType type, char *fmt,...)
*** 93,110 
  	/* fopen() on log_opts.internal might have failed, so check it */
  	if ((type != PG_VERBOSE || log_opts.verbose)  log_opts.internal != NULL)
  	{
! 		/*
! 		 * There's nothing much we can do about it if fwrite fails, but some
! 		 * platforms declare fwrite with warn_unused_result.  Do a little
! 		 * dance with casting to void to shut up the compiler in such cases.
! 

[HACKERS] Fix for pg_upgrade with non-writeable cwd

2012-08-11 Thread Bruce Momjian
An EntpriseDB testing report indicated that pg_upgrade crashes if it
can't write into the current directory.  This only happens in 9.2 and
head, so I have applied the attached patch to fix it.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
new file mode 100644
index 1c71204..2c0dfd6
*** a/contrib/pg_upgrade/util.c
--- b/contrib/pg_upgrade/util.c
*** pg_log(eLogType type, char *fmt,...)
*** 78,84 
  	va_end(args);
  
  	/* PG_VERBOSE is only output in verbose mode */
! 	if (type != PG_VERBOSE || log_opts.verbose)
  	{
  		fwrite(message, strlen(message), 1, log_opts.internal);
  		/* if we are using OVERWRITE_MESSAGE, add newline to log file */
--- 78,85 
  	va_end(args);
  
  	/* PG_VERBOSE is only output in verbose mode */
! 	/* fopen() on log_opts.internal might have failed, so check it */
! 	if ((type != PG_VERBOSE || log_opts.verbose)  log_opts.internal != NULL)
  	{
  		fwrite(message, strlen(message), 1, log_opts.internal);
  		/* if we are using OVERWRITE_MESSAGE, add newline to log file */
-
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Fix for pg_upgrade tablespace function usage

2012-01-24 Thread Bruce Momjian
I have applied the attached patch to git head to fix the new SQL tablespace
location function usage in pg_upgrade to properly check cluster version
numbers, and a fix missing table alias.

I found this problem during testing.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
new file mode 100644
index e8361ce..692cdc2
*** a/contrib/pg_upgrade/info.c
--- b/contrib/pg_upgrade/info.c
*** get_db_infos(ClusterInfo *cluster)
*** 204,210 
/* we don't preserve pg_database.oid so we sort by name */
ORDER BY 2,
/* 9.2 removed the spclocation column */
!   (GET_MAJOR_VERSION(old_cluster.major_version) = 901) ?
t.spclocation : 
pg_catalog.pg_tablespace_location(t.oid) AS spclocation);
  
res = executeQueryOrDie(conn, %s, query);
--- 204,210 
/* we don't preserve pg_database.oid so we sort by name */
ORDER BY 2,
/* 9.2 removed the spclocation column */
!   (GET_MAJOR_VERSION(cluster-major_version) = 901) ?
t.spclocation : 
pg_catalog.pg_tablespace_location(t.oid) AS spclocation);
  
res = executeQueryOrDie(conn, %s, query);
*** get_rel_infos(ClusterInfo *cluster, DbIn
*** 287,293 
/* we preserve pg_class.oid so we sort by it to match old/new */
 ORDER BY 1;,
/* 9.2 removed the spclocation column */
!(GET_MAJOR_VERSION(old_cluster.major_version) = 901) ?
 t.spclocation : 
pg_catalog.pg_tablespace_location(t.oid) AS spclocation,
/* see the comment at the top of old_8_3_create_sequence_script() */
 (GET_MAJOR_VERSION(old_cluster.major_version) = 803) ?
--- 287,293 
/* we preserve pg_class.oid so we sort by it to match old/new */
 ORDER BY 1;,
/* 9.2 removed the spclocation column */
!(GET_MAJOR_VERSION(cluster-major_version) = 901) ?
 t.spclocation : 
pg_catalog.pg_tablespace_location(t.oid) AS spclocation,
/* see the comment at the top of old_8_3_create_sequence_script() */
 (GET_MAJOR_VERSION(old_cluster.major_version) = 803) ?
diff --git a/contrib/pg_upgrade/tablespace.c b/contrib/pg_upgrade/tablespace.c
new file mode 100644
index 11fd9d0..6b61f4b
*** a/contrib/pg_upgrade/tablespace.c
--- b/contrib/pg_upgrade/tablespace.c
*** get_tablespace_paths(void)
*** 53,59 
   spcname != 'pg_global',
/* 9.2 removed the spclocation column */
(GET_MAJOR_VERSION(old_cluster.major_version) = 901) ?
!   t.spclocation : 
pg_catalog.pg_tablespace_location(oid) AS spclocation);
  
res = executeQueryOrDie(conn, %s, query);
  
--- 53,59 
   spcname != 'pg_global',
/* 9.2 removed the spclocation column */
(GET_MAJOR_VERSION(old_cluster.major_version) = 901) ?
!   spclocation : pg_catalog.pg_tablespace_location(oid) 
AS spclocation);
  
res = executeQueryOrDie(conn, %s, query);
  

-- 
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] fix for pg_upgrade

2011-12-08 Thread panam
OK, works now with the recent update.

Thanks

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p5059777.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-30 Thread panam
Great, thanks!

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4856336.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-29 Thread Bruce Momjian
 Alvaro Herrera wrote:
 
  Excerpts from Bruce Momjian's message of miC3A9 sep 28 13:48:28 -0300 
  2011:
   Bruce Momjian wrote:
OK, so it fails for all tables and you are using the newest version.
Thanks for all your work.  I am now guessing that pg_upgrade 9.1.X is
just broken on Windows.
   
Perhaps the variables set by pg_upgrade_support.so are not being passed
into the server variables?  I know pg_upgrade 9.0.X worked on Windows
because EnterpriseDB did extensive testing recently on this.   Has
anyone used pg_upgrade 9.1.X on Windows?
  
   OK, I have a new theory.  postmaster.c processes the -b
   (binary-upgrade) flag by setting a C variable:
  
   case 'b':
   /* Undocumented flag used for binary upgrades */
   IsBinaryUpgrade = true;
   break;
  
   I am now wondering if this variable is not being passed down to the
   sessions during Win32's EXEC_BACKEND.  Looking at the other postmaster
   settings, these set GUC variables, which I assume are passed down.  Can
   someone confirm this?
 
  Well, you could compile it with -DEXEC_BACKEND to test it for yourself.
 
How should this be fixed?
 
  Maybe it should be part of struct BackendParameters.
 
 Thanks.  That's what I did, and tested the failure with -DEXEC_BACKEND,
 and the fix with the patch, which is attached.  I am confident this will
 fix Windows as well.

Applied, and backpatched to 9.1.X.  Thanks for the report.  The fix will
be in 9.1.2.

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-28 Thread panam
Here are all generated log files.

I just removed all other DBs except gnucash (which includes the accounts
table), but the issue also emerges with other DBs.
Upgraded the 9.1 instance to the new build (9.1.1.) as well but this
apparently did not change anything.
PG versions are (including generated logs):
PostgreSQL 9.0.4, compiled by Visual C++ build 1500, 64-bit
PostgreSQL 9.1.0, compiled by Visual C++ build 1500, 64-bit: 
http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.0.zip
pg_upgrade_9.1.0.zip 
PostgreSQL 9.1.1, compiled by Visual C++ build 1500, 64-bit: 
http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.1.zip
pg_upgrade_9.1.1.zip 
I hope that is what you meant with pg_upgrade log file.

Regards,
panam

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4848829.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-28 Thread Bruce Momjian
panam wrote:
 Here are all generated log files.
 
 I just removed all other DBs except gnucash (which includes the accounts
 table), but the issue also emerges with other DBs.
 Upgraded the 9.1 instance to the new build (9.1.1.) as well but this
 apparently did not change anything.
 PG versions are (including generated logs):
 PostgreSQL 9.0.4, compiled by Visual C++ build 1500, 64-bit
 PostgreSQL 9.1.0, compiled by Visual C++ build 1500, 64-bit: 
 http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.0.zip
 pg_upgrade_9.1.0.zip 
 PostgreSQL 9.1.1, compiled by Visual C++ build 1500, 64-bit: 
 http://postgresql.1045698.n5.nabble.com/file/n4848829/pg_upgrade_9.1.1.zip
 pg_upgrade_9.1.1.zip 
 I hope that is what you meant with pg_upgrade log file.

OK, so it fails for all tables and you are using the newest version. 
Thanks for all your work.  I am now guessing that pg_upgrade 9.1.X is
just broken on Windows. 

Perhaps the variables set by pg_upgrade_support.so are not being passed
into the server variables?  I know pg_upgrade 9.0.X worked on Windows
because EnterpriseDB did extensive testing recently on this.   Has
anyone used pg_upgrade 9.1.X on Windows?

As far as a log file, you need you to use '-l log' and email me that
file.

As far as testing, I wonder if we need to load in pg_upgrade_support on
Windows, and rerun some of the pg_dumpall SQL create table statements to
see why the pg_class.oid and others are not getting set.  For example,
this:

-- For binary upgrade, must preserve pg_class oids
SELECT 
binary_upgrade.set_next_heap_pg_class_oid('465783'::pg_catalog.oid);
SELECT 
binary_upgrade.set_next_toast_pg_class_oid('465786'::pg_catalog.oid);
SELECT 
binary_upgrade.set_next_index_pg_class_oid('465788'::pg_catalog.oid);

CREATE TABLE accounts (
guid character varying(32) NOT NULL,
name character varying(2048) NOT NULL,
account_type character varying(2048) NOT NULL,
commodity_guid character varying(32),
commodity_scu integer NOT NULL,
non_std_scu integer NOT NULL,
parent_guid character varying(32),
code character varying(2048),
description character varying(2048),
hidden integer,
placeholder integer
);

should set the accounts pg_class.oid as 465783.  The server will need to
be started with -b and this will disable autovacuum.  Can someone on
Windows try this?

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-28 Thread Bruce Momjian
Bruce Momjian wrote:
 OK, so it fails for all tables and you are using the newest version. 
 Thanks for all your work.  I am now guessing that pg_upgrade 9.1.X is
 just broken on Windows. 
 
 Perhaps the variables set by pg_upgrade_support.so are not being passed
 into the server variables?  I know pg_upgrade 9.0.X worked on Windows
 because EnterpriseDB did extensive testing recently on this.   Has
 anyone used pg_upgrade 9.1.X on Windows?

OK, I have a new theory.  postmaster.c processes the -b
(binary-upgrade) flag by setting a C variable:

case 'b':
/* Undocumented flag used for binary upgrades */
IsBinaryUpgrade = true;
break;

I am now wondering if this variable is not being passed down to the
sessions during Win32's EXEC_BACKEND.  Looking at the other postmaster
settings, these set GUC variables, which I assume are passed down.  Can
someone confirm this?  How should this be fixed?

FYI, the binary-upgrade set() functions will not operate unless the -b
option is enabled, which explains the failure the reporter is seeing.

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-28 Thread panam
Hi Bruce,

here is the file you asked for: 
http://postgresql.1045698.n5.nabble.com/file/n4850735/pg_upgrade_logfile.txt
pg_upgrade_logfile.txt 

I guess you are not addressing me here, right?
 The server will need to 
 be started with -b and this will disable autovacuum.  Can someone on 
 Windows try this?

Thanks
panam 

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4850735.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-28 Thread Bruce Momjian
panam wrote:
 Hi Bruce,
 
 here is the file you asked for: 
 http://postgresql.1045698.n5.nabble.com/file/n4850735/pg_upgrade_logfile.txt
 pg_upgrade_logfile.txt 
 

OK, I see it using -b to pg_ctl:

C:\Program Files\PostgreSQL\9.1\bin/pg_ctl -w -l nul -D 
D:\applications\postgres\9.1 -o -p 5432 -b start  nul 21

What I have to find out is whether this is passed to the individual
session processes.  I guess is no.

 I guess you are not addressing me here, right?
  The server will need to 
  be started with -b and this will disable autovacuum.  Can someone on 
  Windows try this?

No, not really.  I think it is a software bug and I need guidance about
a solution.

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-28 Thread Alvaro Herrera

Excerpts from Bruce Momjian's message of mié sep 28 13:48:28 -0300 2011:
 Bruce Momjian wrote:
  OK, so it fails for all tables and you are using the newest version. 
  Thanks for all your work.  I am now guessing that pg_upgrade 9.1.X is
  just broken on Windows. 
  
  Perhaps the variables set by pg_upgrade_support.so are not being passed
  into the server variables?  I know pg_upgrade 9.0.X worked on Windows
  because EnterpriseDB did extensive testing recently on this.   Has
  anyone used pg_upgrade 9.1.X on Windows?
 
 OK, I have a new theory.  postmaster.c processes the -b
 (binary-upgrade) flag by setting a C variable:
 
 case 'b':
 /* Undocumented flag used for binary upgrades */
 IsBinaryUpgrade = true;
 break;
 
 I am now wondering if this variable is not being passed down to the
 sessions during Win32's EXEC_BACKEND.  Looking at the other postmaster
 settings, these set GUC variables, which I assume are passed down.  Can
 someone confirm this?

Well, you could compile it with -DEXEC_BACKEND to test it for yourself.

  How should this be fixed?

Maybe it should be part of struct BackendParameters.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] fix for pg_upgrade

2011-09-28 Thread Bruce Momjian
Alvaro Herrera wrote:
 
 Excerpts from Bruce Momjian's message of mi?? sep 28 13:48:28 -0300 2011:
  Bruce Momjian wrote:
   OK, so it fails for all tables and you are using the newest version. 
   Thanks for all your work.  I am now guessing that pg_upgrade 9.1.X is
   just broken on Windows. 
   
   Perhaps the variables set by pg_upgrade_support.so are not being passed
   into the server variables?  I know pg_upgrade 9.0.X worked on Windows
   because EnterpriseDB did extensive testing recently on this.   Has
   anyone used pg_upgrade 9.1.X on Windows?
  
  OK, I have a new theory.  postmaster.c processes the -b
  (binary-upgrade) flag by setting a C variable:
  
  case 'b':
  /* Undocumented flag used for binary upgrades */
  IsBinaryUpgrade = true;
  break;
  
  I am now wondering if this variable is not being passed down to the
  sessions during Win32's EXEC_BACKEND.  Looking at the other postmaster
  settings, these set GUC variables, which I assume are passed down.  Can
  someone confirm this?
 
 Well, you could compile it with -DEXEC_BACKEND to test it for yourself.
 
   How should this be fixed?
 
 Maybe it should be part of struct BackendParameters.

Thanks.  That's what I did, and tested the failure with -DEXEC_BACKEND,
and the fix with the patch, which is attached.  I am confident this will
fix Windows as well.

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

  + It's impossible for everything to be true. +
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 94b57fa..0a84d97
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** typedef struct
*** 433,438 
--- 433,439 
  	TimestampTz PgStartTime;
  	TimestampTz PgReloadTime;
  	bool		redirection_done;
+ 	bool		IsBinaryUpgrade;
  #ifdef WIN32
  	HANDLE		PostmasterHandle;
  	HANDLE		initial_signal_pipe;
*** save_backend_variables(BackendParameters
*** 4653,4658 
--- 4654,4660 
  	param-PgReloadTime = PgReloadTime;
  
  	param-redirection_done = redirection_done;
+ 	param-IsBinaryUpgrade = IsBinaryUpgrade;
  
  #ifdef WIN32
  	param-PostmasterHandle = PostmasterHandle;
*** restore_backend_variables(BackendParamet
*** 4874,4879 
--- 4876,4882 
  	PgReloadTime = param-PgReloadTime;
  
  	redirection_done = param-redirection_done;
+ 	IsBinaryUpgrade = param-IsBinaryUpgrade;
  
  #ifdef WIN32
  	PostmasterHandle = param-PostmasterHandle;

-- 
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] fix for pg_upgrade

2011-09-27 Thread panam
Hi Bruce,

here is the whole dump (old DB):
http://postgresql.1045698.n5.nabble.com/file/n4844725/dump.txt dump.txt 

Regards,
panam

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4844725.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-27 Thread Bruce Momjian
panam wrote:
 Hi Bruce,
 
 here is the whole dump (old DB):
 http://postgresql.1045698.n5.nabble.com/file/n4844725/dump.txt dump.txt 

Wow, that is interesting.  I see this in the dump output:

-- For binary upgrade, must preserve relfilenodes
SELECT 
binary_upgrade.set_next_heap_relfilenode('465783'::pg_catalog.oid);
SELECT 
binary_upgrade.set_next_toast_relfilenode('465786'::pg_catalog.oid);
SELECT 
binary_upgrade.set_next_index_relfilenode('465788'::pg_catalog.oid);

CREATE TABLE accounts (
guid character varying(32) NOT NULL,
name character varying(2048) NOT NULL,
account_type character varying(2048) NOT NULL,
commodity_guid character varying(32),
commodity_scu integer NOT NULL,
non_std_scu integer NOT NULL,
parent_guid character varying(32),
code character varying(2048),
description character varying(2048),
hidden integer,
placeholder integer
);

and it is clearly saying the oid/relfilenode should be 465783, but your
9.1 query shows:

C:\Program Files\PostgreSQL\9.1\binpsql -c select * from pg_class 
where oid = 465783 or oid = 16505; -p 5433 -U postgres
 relname  | relnamespace | reltype | reloftype | relowner | relam | 
relfilenode | reltablespace | relpages | reltuples | reltoastrelid | 
reltoastidxid | relhasindex | relisshared | relpersistence | relkind | relnatts 
| relchecks | relhasoids | relhaspkey | relhasrules | relhastriggers | 
relhassubclass | relfrozenxid | relacl | reloptions 

--+--+-+---+--+---+-+---+--+---+---+---+-+-++-+--+---+++-+++--++
 accounts | 2200 |   16507 | 0 |16417 | 0 | 
  16505 | 0 |0 | 0 | 16508 | 0 
| t   | f   | p  | r   |   11 | 0 | 
f  | t  | f   | f  | f  |  
3934366 || 
(1 row)

and 9.0 says correctly 465783:

C:\Program Files\PostgreSQL\9.0\binpsql -c select * from pg_class 
where oid = 465783 or oid = 16505; -p 5432 -U postgres
 relname  | relnamespace | reltype | reloftype | relowner | relam | 
relfilenode | reltablespace | relpages | reltuples | reltoastrelid | 
reltoastidxid | relhasindex | relisshared | relistemp | relkind | relnatts | 
relchecks | relhasoids | relhaspkey | relhasexclusion | relhasrules | 
relhastriggers | relhassubclass | relfrozenxid | relacl | reloptions 

--+--+-+---+--+---+-+---+--+---+---+---+-+-+---+-+--+---+++-+-+++--++
 accounts |   465781 |  465785 | 0 |   456619 | 0 | 
 465783 | 0 |3 |   122 |465786 | 0 
| t   | f   | f | r   |   11 | 0 | f
  | t  | f   | f   | f  | f 
 |  3934366 || 
(1 row)

It is as though the system ignoring the set_next_heap_relfilenode()
call, but I don't see how that could happen.  I don't see any other
'accounts' table in that dump.

My only guess at this point is that somehow the -b/IsBinaryUpgrade flag
is not being processed or regognized, and hence the binary_upgrade 'set'
routines are not working.

Is this 9.1 final or later?  Can you turn on debug mode and send me the
pg_upgrade log file that is generated?  I am going go look for the
pg_ctl -o '-b' flag.  Are all databases/objects failing or just this
one?

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-26 Thread Bruce Momjian
panam wrote:
 Hi Bruce,
 
 on the old DB I've got 465783 as oid whereas on the new one it is 16505.
 
 is not in the dump file (old db), even 16385 (i guess this is a typo here)
 or 16505 are not.
 The only line in which 465783 could be found is

I need to see the lines after this.

 Is that enough information or should I send the whole dump? That's a bit of
 work as I have to expunge some sensitive schema data, or is there a
 meaningful way to just do the dump for a single db?

You can do:

pg_dump --binary-upgrade --schema-only dbname


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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-26 Thread panam
Hi Bruce,

on the old DB I've got 465783 as oid whereas on the new one it is 16505.

is not in the dump file (old db), even 16385 (i guess this is a typo here)
or 16505 are not.
The only line in which 465783 could be found is

Is that enough information or should I send the whole dump? That's a bit of
work as I have to expunge some sensitive schema data, or is there a
meaningful way to just do the dump for a single db?

Thanks  regards,
panam

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4843289.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-25 Thread panam
OK, i started once again:


I hope the following is the correct way of querying the table corresponding
to a relid:









--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4838427.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

-- 
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] fix for pg_upgrade

2011-09-25 Thread Bruce Momjian
panam wrote:
 OK, i started once again:
 
 
 I hope the following is the correct way of querying the table corresponding
 to a relid:
 
 
 
 
 
 
 
 
 
 --
 View this message in context: 
 http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4838427.html

Yes, that is very close to what I needed.  Ideally you would have
included the oid from pg_class:

select oid, * from pg_class where oid = 465783 or oid = 16505

Can you supply that?

Also can you email me privately the following output from the old
database?  It should only be the schema and not your data:

pg_dumpall --schema-only --binary-upgrade

I am looking for something like this in the file:

-- For binary upgrade, must preserve pg_class oids
SELECT 
binary_upgrade.set_next_heap_pg_class_oid('16385'::pg_catalog.oid);

CREATE TABLE test (
x integer
);

but for your case it would be the 'accounts' file.  You can email just
those lines if you want, and that you can probably email to hackers. 
Thanks.

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-24 Thread Bruce Momjian
\panam wrote:
 Hi, just tried to upgrade from 9.0 to 9.1 and got this error during
 pg_upgrade :
 Mismatch of relation id: database xyz, old relid 465783, new relid 16494
 It seems, I get this error on every table as I got it on another table
 (which I did not need and deleted) before as well. Schmemas seem to be
 migrated but the content is missing.
 
 I am using Windows 7 64bit (both PG servers are 64 bit as well), everthing
 on the same machine.

Sorry for the delay in replying.  It is odd you got a mismatch of relids
because pg_upgrade is supposed to preserve all of those.  Can you do a
query to find out what table is relid of 465783 on the old cluster?

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

  + It's impossible for everything to be true. +

-- 
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] fix for pg_upgrade

2011-09-13 Thread panam
Hi, just tried to upgrade from 9.0 to 9.1 and got this error during
pg_upgrade :
Mismatch of relation id: database xyz, old relid 465783, new relid 16494
It seems, I get this error on every table as I got it on another table
(which I did not need and deleted) before as well. Schmemas seem to be
migrated but the content is missing.

I am using Windows 7 64bit (both PG servers are 64 bit as well), everthing
on the same machine.

Any ideas?
Thanks  regards
panam

--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/fix-for-pg-upgrade-tp3411128p4798957.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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


[HACKERS] Fix for pg_upgrade user flag

2011-05-07 Thread Bruce Momjian
The attached, applied patch checks that the pg_upgrade user specified is
a super-user.  It also reports the error message when the post-pg_ctl
connection fails.

This was prompted by a private bug report from EnterpriseDB.

-- 
  Bruce Momjian  br...@momjian.ushttp://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
new file mode 100644
index 35b178e..26dec39
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** static void check_new_cluster_is_empty(v
*** 15,20 
--- 15,21 
  static void check_old_cluster_has_new_cluster_dbs(void);
  static void check_locale_and_encoding(ControlData *oldctrl,
  		  ControlData *newctrl);
+ static void check_is_super_user(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
  
*** check_old_cluster(bool live_check,
*** 63,69 
  	/*
  	 * Check for various failure cases
  	 */
! 
  	check_for_reg_data_type_usage(old_cluster);
  	check_for_isn_and_int8_passing_mismatch(old_cluster);
  
--- 64,70 
  	/*
  	 * Check for various failure cases
  	 */
! 	check_is_super_user(old_cluster);
  	check_for_reg_data_type_usage(old_cluster);
  	check_for_isn_and_int8_passing_mismatch(old_cluster);
  
*** create_script_for_old_cluster_deletion(
*** 473,478 
--- 474,505 
  
  
  /*
+  *	check_is_super_user()
+  *
+  *	Make sure we are the super-user.
+  */
+ static void
+ check_is_super_user(ClusterInfo *cluster)
+ {
+ 	PGresult   *res;
+ 	PGconn	   *conn = connectToServer(cluster, template1);
+ 
+ 	/* Can't use pg_authid because only superusers can view it. */
+ 	res = executeQueryOrDie(conn,
+ 			SELECT rolsuper 
+ 			FROM pg_catalog.pg_roles 
+ 			WHERE rolname = current_user);
+ 
+ 	if (PQntuples(res) != 1 || strcmp(PQgetvalue(res, 0, 0), t) != 0)
+ 		pg_log(PG_FATAL, the database user is not a superuser\n);
+ 
+ 	PQclear(res);
+ 
+ 	PQfinish(conn);
+ }
+ 
+ 
+ /*
   *	check_for_isn_and_int8_passing_mismatch()
   *
   *	/contrib/isn relies on data type int8, and in 8.4 int8 can now be passed
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index 9a55075..d6efe9a
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** connectToServer(ClusterInfo *cluster, co
*** 27,33 
  
  	if (conn == NULL || PQstatus(conn) != CONNECTION_OK)
  	{
! 		pg_log(PG_REPORT, Connection to database failed: %s\n,
  			   PQerrorMessage(conn));
  
  		if (conn)
--- 27,33 
  
  	if (conn == NULL || PQstatus(conn) != CONNECTION_OK)
  	{
! 		pg_log(PG_REPORT, connection to database failed: %s\n,
  			   PQerrorMessage(conn));
  
  		if (conn)
*** start_postmaster(ClusterInfo *cluster)
*** 189,195 
  	if ((conn = get_db_conn(cluster, template1)) == NULL ||
  		PQstatus(conn) != CONNECTION_OK)
  	{
! 		if (conn)
  			PQfinish(conn);
  		pg_log(PG_FATAL, unable to connect to %s postmaster started with the command: %s\n
  			   Perhaps pg_hba.conf was not set to \trust\.\n,
--- 189,197 
  	if ((conn = get_db_conn(cluster, template1)) == NULL ||
  		PQstatus(conn) != CONNECTION_OK)
  	{
! 		pg_log(PG_REPORT, \nconnection to database failed: %s\n,
! 			   PQerrorMessage(conn));
!  		if (conn)
  			PQfinish(conn);
  		pg_log(PG_FATAL, unable to connect to %s postmaster started with the command: %s\n
  			   Perhaps pg_hba.conf was not set to \trust\.\n,

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Robert Haas
On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian br...@momjian.us wrote:
 The attached, applied patch checks that the pg_upgrade user specified is
 a super-user.  It also reports the error message when the post-pg_ctl
 connection fails.

 This was prompted by a private bug report from EnterpriseDB.

It strikes me that it's fairly crazy to think you're going to be able
to catch all the possible errors the server might throw this way.
Don't we need some way of letting the actual server errors leak out?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Robert Haas
On Sat, May 7, 2011 at 9:50 AM, Robert Haas robertmh...@gmail.com wrote:
 On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian br...@momjian.us wrote:
 The attached, applied patch checks that the pg_upgrade user specified is
 a super-user.  It also reports the error message when the post-pg_ctl
 connection fails.

 This was prompted by a private bug report from EnterpriseDB.

 It strikes me that it's fairly crazy to think you're going to be able
 to catch all the possible errors the server might throw this way.
 Don't we need some way of letting the actual server errors leak out?

Or, hmm.  Maybe you just did that.  If so, never mind.  :-)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Bruce Momjian
Robert Haas wrote:
 On Sat, May 7, 2011 at 9:50 AM, Robert Haas robertmh...@gmail.com wrote:
  On Sat, May 7, 2011 at 8:56 AM, Bruce Momjian br...@momjian.us wrote:
  The attached, applied patch checks that the pg_upgrade user specified is
  a super-user. ?It also reports the error message when the post-pg_ctl
  connection fails.
 
  This was prompted by a private bug report from EnterpriseDB.
 
  It strikes me that it's fairly crazy to think you're going to be able
  to catch all the possible errors the server might throw this way.
  Don't we need some way of letting the actual server errors leak out?
 
 Or, hmm.  Maybe you just did that.  If so, never mind.  :-)

What I did was to report the errors of our first database probe after we
started the server --- for some reason, that code was not reporting the
libpq error message, while all other failed connections did.

The second change was to only run pg_upgrade as a database super-user,
and hopefully that will avoid odd pg_dump error messages.  

One question I have is why we even bother to allow the database username
to be specified?  Shouldn't we just hard-code that to 'postgres'?  Is
there any reason to allow another username to be used?  You can't drop
the postgres user but you can remove super-user permissions from it so
maybe we have to continue allowing it:

postgres= drop user postgres;
ERROR:  cannot drop role postgres because it is required by the 
database system
postgres= alter user postgres nosuperuser;
ALTER ROLE

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

  + It's impossible for everything to be true. +

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 One question I have is why we even bother to allow the database username
 to be specified?  Shouldn't we just hard-code that to 'postgres'?

Only if you want to render pg_upgrade unusable by a significant fraction
of people.  postgres is not the hard wired name of the bootstrap
superuser.

regards, tom lane

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  One question I have is why we even bother to allow the database username
  to be specified?  Shouldn't we just hard-code that to 'postgres'?
 
 Only if you want to render pg_upgrade unusable by a significant fraction
 of people.  postgres is not the hard wired name of the bootstrap
 superuser.

I was really wondering if I should be using that hard-coded name, rather
than allowing the user to supply it.  They have to compile in a
different name, and I assume that name is accessible somewhere.

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

  + It's impossible for everything to be true. +

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Kevin Grittner
 Bruce Momjian  wrote:
 Tom Lane wrote:
 Bruce Momjian  writes:
 One question I have is why we even bother to allow the database
 username to be specified? Shouldn't we just hard-code that to
 'postgres'?

 Only if you want to render pg_upgrade unusable by a significant
 fraction of people. postgres is not the hard wired name of the
 bootstrap superuser.
 
 I was really wondering if I should be using that hard-coded name,
 rather than allowing the user to supply it. They have to compile in
 a different name, and I assume that name is accessible somewhere.
 
At home, on my ubuntu machine, I built and ran initdb without
specifying a superuser.  It used my OS login of kevin.  Then,
 
test=# \du
 List of roles
 Role name |   Attributes   | Member
of 
---++
---
 kevin | Superuser, Create role, Create DB, Replication | {}

test=# create user xxx superuser;
CREATE ROLE
test=# \c test xxx
You are now connected to database test as user xxx.
test=# alter user kevin rename to yyy;
ALTER ROLE
test=# \du
 List of roles
 Role name |   Attributes   | Member
of 
---++
---
 xxx   | Superuser, Replication | {}
 yyy   | Superuser, Create role, Create DB, Replication | {}
 
If I run pg_upgrade now, what will it pick?  How?
 
-Kevin

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Peter Eisentraut
On lör, 2011-05-07 at 13:50 -0400, Bruce Momjian wrote:
 I was really wondering if I should be using that hard-coded name,
 rather than allowing the user to supply it.  They have to compile in a
 different name, and I assume that name is accessible somewhere.

postgres is not compiled in.  It's whatever user you run initdb under.
In particular, in the regression tests, it is probably not postgres.


-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Bruce Momjian
Kevin Grittner wrote:
  Bruce Momjian  wrote:
  Tom Lane wrote:
  Bruce Momjian  writes:
  One question I have is why we even bother to allow the database
  username to be specified? Shouldn't we just hard-code that to
  'postgres'?
 
  Only if you want to render pg_upgrade unusable by a significant
  fraction of people. postgres is not the hard wired name of the
  bootstrap superuser.
  
  I was really wondering if I should be using that hard-coded name,
  rather than allowing the user to supply it. They have to compile in
  a different name, and I assume that name is accessible somewhere.
  
 At home, on my ubuntu machine, I built and ran initdb without
 specifying a superuser.  It used my OS login of kevin.  Then,
  
 test=# \du
  List of roles
  Role name |   Attributes   | Member
 of 
 ---++
 ---
  kevin | Superuser, Create role, Create DB, Replication | {}
 
 test=# create user xxx superuser;
 CREATE ROLE
 test=# \c test xxx
 You are now connected to database test as user xxx.
 test=# alter user kevin rename to yyy;
 ALTER ROLE
 test=# \du
  List of roles
  Role name |   Attributes   | Member
 of 
 ---++
 ---
  xxx   | Superuser, Replication | {}
  yyy   | Superuser, Create role, Create DB, Replication | {}
  
 If I run pg_upgrade now, what will it pick?  How?

Good point --- you would need the user flag in pg_upgrade.  Thanks.

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

  + It's impossible for everything to be true. +

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Bruce Momjian
Peter Eisentraut wrote:
 On l?r, 2011-05-07 at 13:50 -0400, Bruce Momjian wrote:
  I was really wondering if I should be using that hard-coded name,
  rather than allowing the user to supply it.  They have to compile in a
  different name, and I assume that name is accessible somewhere.
 
 postgres is not compiled in.  It's whatever user you run initdb under.
 In particular, in the regression tests, it is probably not postgres.

Thanks.  I get confused because the 'postgres' database is hardcoded in,
but not the username.  Not sure why I am so easily confused.

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

  + It's impossible for everything to be true. +

-- 
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] Fix for pg_upgrade user flag

2011-05-07 Thread Andrew Dunstan



On 05/07/2011 06:48 PM, Bruce Momjian wrote:

postgres is not compiled in.  It's whatever user you run initdb under.
In particular, in the regression tests, it is probably not postgres.

Thanks.  I get confused because the 'postgres' database is hardcoded in,
but not the username.  Not sure why I am so easily confused.



There is a requirement for a known database name, but no requirement for 
a known superuser name.


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


[HACKERS] Fix for pg_upgrade with extra new cluster databases

2011-04-19 Thread Bruce Momjian
The attached, applied patch adds a check to throw a pg_upgrade error
during the check phase, rather than during the post-check upgrade phase.

Specifically, if someone removes the 'postgres' database from the old
cluster and the new cluster has a 'postgres' database, the number of
databases will not match.  We actually could upgrade such a setup, but
it would violate the 1-to-1 mapping of database counts, so we throw an
error instead.

Previously they got an error during the upgrade, and not at the check
stage; PG 9.0.4 does the same.

-- 
  Bruce Momjian  br...@momjian.ushttp://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
new file mode 100644
index 7472440..469bbdb
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
***
*** 11,17 
  
  
  static void set_locale_and_encoding(ClusterInfo *cluster);
! static void check_new_db_is_empty(void);
  static void check_locale_and_encoding(ControlData *oldctrl,
  		  ControlData *newctrl);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
--- 11,18 
  
  
  static void set_locale_and_encoding(ClusterInfo *cluster);
! static void check_new_cluster_is_empty(void);
! static void check_old_cluster_has_new_cluster_dbs(void);
  static void check_locale_and_encoding(ControlData *oldctrl,
  		  ControlData *newctrl);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
*** check_new_cluster(void)
*** 112,118 
  {
  	set_locale_and_encoding(new_cluster);
  
! 	check_new_db_is_empty();
  
  	check_loadable_libraries();
  
--- 113,122 
  {
  	set_locale_and_encoding(new_cluster);
  
! 	get_db_and_rel_infos(new_cluster);
! 
! 	check_new_cluster_is_empty();
! 	check_old_cluster_has_new_cluster_dbs();
  
  	check_loadable_libraries();
  
*** check_locale_and_encoding(ControlData *o
*** 341,352 
  
  
  static void
! check_new_db_is_empty(void)
  {
  	int			dbnum;
- 	bool		found = false;
- 
- 	get_db_and_rel_infos(new_cluster);
  
  	for (dbnum = 0; dbnum  new_cluster.dbarr.ndbs; dbnum++)
  	{
--- 345,353 
  
  
  static void
! check_new_cluster_is_empty(void)
  {
  	int			dbnum;
  
  	for (dbnum = 0; dbnum  new_cluster.dbarr.ndbs; dbnum++)
  	{
*** check_new_db_is_empty(void)
*** 358,372 
  		{
  			/* pg_largeobject and its index should be skipped */
  			if (strcmp(rel_arr-rels[relnum].nspname, pg_catalog) != 0)
! 			{
! found = true;
! break;
! 			}
  		}
  	}
  
! 	if (found)
! 		pg_log(PG_FATAL, New cluster is not empty; exiting\n);
  }
  
  
--- 359,394 
  		{
  			/* pg_largeobject and its index should be skipped */
  			if (strcmp(rel_arr-rels[relnum].nspname, pg_catalog) != 0)
! pg_log(PG_FATAL, New cluster database \%s\ is not empty\n,
! 	new_cluster.dbarr.dbs[dbnum].db_name);
  		}
  	}
  
! }
! 
! 
! /*
!  *	If someone removes the 'postgres' database from the old cluster and
!  *	the new cluster has a 'postgres' database, the number of databases
!  *	will not match.  We actually could upgrade such a setup, but it would
!  *	violate the 1-to-1 mapping of database counts, so we throw an error
!  *	instead.
!  */
! static void
! check_old_cluster_has_new_cluster_dbs(void)
! {
! 	int			old_dbnum, new_dbnum;
! 
! 	for (new_dbnum = 0; new_dbnum  new_cluster.dbarr.ndbs; new_dbnum++)
! 	{
! 		for (old_dbnum = 0; old_dbnum  old_cluster.dbarr.ndbs; old_dbnum++)
! 			if (strcmp(old_cluster.dbarr.dbs[old_dbnum].db_name,
! new_cluster.dbarr.dbs[new_dbnum].db_name) == 0)
! break;
! 		if (old_dbnum == old_cluster.dbarr.ndbs)
! 			pg_log(PG_FATAL, New cluster database \%s\ does not exist in the old cluster\n,
! new_cluster.dbarr.dbs[new_dbnum].db_name);
! 	}
  }
  
  
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index 6fb336c..9a0a3ac
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*** transfer_all_new_dbs(DbInfoArr *old_db_a
*** 37,48 
  
  	prep_status(Restoring user relation files\n);
  
- 	/*
- 	 *	If the user removed the 'postgres' database from the old cluster,
- 	 *	this will cause the database counts to not match and throw an error.
- 	 *	We could allow this to work because the new database is empty (we
- 	 *	checked), but we don't.
- 	 */
  	if (old_db_arr-ndbs != new_db_arr-ndbs)
  		pg_log(PG_FATAL, old and new clusters have a different number of databases\n);
  
--- 37,42 

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


[HACKERS] Fix for pg_upgrade

2011-04-13 Thread Bruce Momjian
The attached patches fixes a pg_upgrade crash in 9.0 caused by a new
cluster database that doesn't exist in the old cluster;  instead throw
an error.  This was reported to me by EnterpriseDB testing staff.  This
bug does not exist in git head.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/relfilenode.c b/contrib/pg_upgrade/relfilenode.c
new file mode 100644
index b20ca20..fc36968
*** a/contrib/pg_upgrade/relfilenode.c
--- b/contrib/pg_upgrade/relfilenode.c
*** transfer_all_new_dbs(migratorContext *ct
*** 46,51 
--- 46,55 
  		int			n_maps;
  		pageCnvCtx *pageConverter = NULL;
  
+ 		if (!old_db)
+ 			pg_log(ctx, PG_FATAL,
+ 			   the new cluster database %s was not found in the old cluster\n, new_db-db_name);
+ 		
  		n_maps = 0;
  		mappings = gen_db_file_maps(ctx, old_db, new_db, n_maps, old_pgdata,
  	new_pgdata);

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


[HACKERS] Fix for pg_upgrade discriptor leaks

2011-03-08 Thread Bruce Momjian
I have applied the attached patch to fix pg_upgrade file descriptor
leaks in error paths.

-- 
  Bruce Momjian  br...@momjian.ushttp://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 9ef63ec..1c4847a 100644
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** check_for_isn_and_int8_passing_mismatch(
*** 515,523 
  		PQfinish(conn);
  	}
  
  	if (found)
  	{
- 		fclose(script);
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains \/contrib/isn\ functions\n
--- 515,525 
  		PQfinish(conn);
  	}
  
+ 	if (script)
+ 			fclose(script);
+ 
  	if (found)
  	{
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains \/contrib/isn\ functions\n
*** check_for_reg_data_type_usage(ClusterInf
*** 616,624 
  		PQfinish(conn);
  	}
  
  	if (found)
  	{
- 		fclose(script);
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains one of the reg* data types in\n
--- 618,628 
  		PQfinish(conn);
  	}
  
+ 	if (script)
+ 		fclose(script);
+ 
  	if (found)
  	{
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains one of the reg* data types in\n
diff --git a/contrib/pg_upgrade/file.c b/contrib/pg_upgrade/file.c
index deaca46..803e0a0 100644
*** a/contrib/pg_upgrade/file.c
--- b/contrib/pg_upgrade/file.c
*** pg_scandir_internal(const char *dirname,
*** 302,308 
--- 302,311 
  		(size_t) ((name_num + 1) * sizeof(struct dirent *)));
  
  			if (*namelist == NULL)
+ 			{
+ closedir(dirdesc);
  return -1;
+ 			}
  
  			entrysize = sizeof(struct dirent) - sizeof(direntry-d_name) +
  strlen(direntry-d_name) + 1;
*** pg_scandir_internal(const char *dirname,
*** 310,316 
--- 313,322 
  			(*namelist)[name_num] = (struct dirent *) malloc(entrysize);
  
  			if ((*namelist)[name_num] == NULL)
+ 			{
+ closedir(dirdesc);
  return -1;
+ 			}
  
  			memcpy((*namelist)[name_num], direntry, entrysize);
  
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index c7684f8..84bd03e 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** get_major_server_version(ClusterInfo *cl
*** 144,154 
  	if (fscanf(version_fd, %63s, cluster-major_version_str) == 0 ||
  		sscanf(cluster-major_version_str, %d.%d, integer_version,
  			   fractional_version) != 2)
- 	{
  		pg_log(PG_FATAL, could not get version from %s\n, datadir);
! 		fclose(version_fd);
! 		return 0;
! 	}
  
  	return (100 * integer_version + fractional_version) * 100;
  }
--- 144,152 
  	if (fscanf(version_fd, %63s, cluster-major_version_str) == 0 ||
  		sscanf(cluster-major_version_str, %d.%d, integer_version,
  			   fractional_version) != 2)
  		pg_log(PG_FATAL, could not get version from %s\n, datadir);
! 
! 	fclose(version_fd);
  
  	return (100 * integer_version + fractional_version) * 100;
  }
diff --git a/contrib/pg_upgrade/version.c b/contrib/pg_upgrade/version.c
index e321538..8ba7e98 100644
*** a/contrib/pg_upgrade/version.c
--- b/contrib/pg_upgrade/version.c
*** new_9_0_populate_pg_largeobject_metadata
*** 62,71 
  		PQfinish(conn);
  	}
  
  	if (found)
  	{
- 		if (!check_mode)
- 			fclose(script);
  		report_status(PG_WARNING, warning);
  		if (check_mode)
  			pg_log(PG_WARNING, \n
--- 62,72 
  		PQfinish(conn);
  	}
  
+ 	if (script)
+ 		fclose(script);
+ 
  	if (found)
  	{
  		report_status(PG_WARNING, warning);
  		if (check_mode)
  			pg_log(PG_WARNING, \n
diff --git a/contrib/pg_upgrade/version_old_8_3.c b/contrib/pg_upgrade/version_old_8_3.c
index 5b226b2..3ec4b59 100644
*** a/contrib/pg_upgrade/version_old_8_3.c
--- b/contrib/pg_upgrade/version_old_8_3.c
*** old_8_3_check_for_name_data_type_usage(C
*** 87,95 
  		PQfinish(conn);
  	}
  
  	if (found)
  	{
- 		fclose(script);
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains the \name\ data type in\n
--- 87,97 
  		PQfinish(conn);
  	}
  
+ 	if (script)
+ 		fclose(script);
+ 
  	if (found)
  	{
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains the \name\ data type in\n
*** old_8_3_check_for_tsquery_usage(ClusterI
*** 175,183 
  		PQfinish(conn);
  	}
  
  	if (found)
  	{
- 		fclose(script);
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains the \tsquery\ data type.\n
--- 177,187 
  		PQfinish(conn);
  	}
  
+ 	if (script)
+ 		fclose(script);
+ 
  	if (found)
  	{
  		pg_log(PG_REPORT, fatal\n);
  		pg_log(PG_FATAL,
  			   | Your installation contains the \tsquery\ data type.\n
*** old_8_3_rebuild_tsvector_tables(ClusterI
*** 314,323 
  		

Re: [HACKERS] Fix for pg_upgrade discriptor leaks

2011-03-08 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 I have applied the attached patch to fix pg_upgrade file descriptor
 leaks in error paths.

It seems rather pointless to spend code closing descriptors immediately
before a fatal exit.

regards, tom lane

-- 
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] Fix for pg_upgrade discriptor leaks

2011-03-08 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  I have applied the attached patch to fix pg_upgrade file descriptor
  leaks in error paths.
 
 It seems rather pointless to spend code closing descriptors immediately
 before a fatal exit.

Well, it is not before a fatal but rather before it returns -1, which
might fatal or might not.  I also had code in to close file descriptors
that was a little too tricky about using a single variable to indicate
two things so I cleaned it up.  I got a private email report about these
so obviously they were confusing.

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

  + It's impossible for everything to be true. +

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


[HACKERS] fix for pg_upgrade

2011-03-05 Thread Bruce Momjian
I got a report from someone using pg_upgrade coming from PG 8.3 ---
turns out we didn't rename toast tables to match the new relfilenode in
pre-8.4, so the attached applied patch avoids the check for these cases.
This check is new in pg_upgrade for 9.1.

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

  + It's impossible for everything to be true. +
commit 9e5bed2df1693a46dfaed862d7462ba2379f8f79
Author: Bruce Momjian br...@momjian.us
Date:   Sat Mar 5 21:12:21 2011 -0500

Restructure pg_upgrade checks because pre-8.4 Postgres did not rename
toast file names to match the new relfilenode.

diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
index a502507..ad7edc4 100644
*** a/contrib/pg_upgrade/info.c
--- b/contrib/pg_upgrade/info.c
*** gen_db_file_maps(DbInfo *old_db, DbInfo 
*** 51,59 
  		RelInfo*new_rel = new_db-rel_arr.rels[relnum];
  
  		if (old_rel-reloid != new_rel-reloid)
! 			pg_log(PG_FATAL, mismatch of relation id: database \%s\, old relid %d, new relid %d\n,
  			old_db-db_name, old_rel-reloid, new_rel-reloid);
! 	
  		create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
  old_rel, new_rel, maps + num_maps);
  		num_maps++;
--- 51,68 
  		RelInfo*new_rel = new_db-rel_arr.rels[relnum];
  
  		if (old_rel-reloid != new_rel-reloid)
! 			pg_log(PG_FATAL, Mismatch of relation id: database \%s\, old relid %d, new relid %d\n,
  			old_db-db_name, old_rel-reloid, new_rel-reloid);
! 
! 		/* toast names were not renamed to match their relfilenodes in pre-8.4 */
! 		if (GET_MAJOR_VERSION(old_cluster.major_version) = 804 
! 			(strcmp(old_rel-nspname, new_rel-nspname) != 0 ||
! 		 strcmp(old_rel-relname, new_rel-relname) != 0))
! 			pg_log(PG_FATAL, Mismatch of relation names: database \%s\, 
! old rel %s.%s, new rel %s.%s\n,
! old_db-db_name, old_rel-nspname, old_rel-relname,
! new_rel-nspname, new_rel-relname);
! 
  		create_rel_filename_map(old_pgdata, new_pgdata, old_db, new_db,
  old_rel, new_rel, maps + num_maps);
  		num_maps++;
*** create_rel_filename_map(const char *old_
*** 104,115 
  	/* new_relfilenode will match old and new pg_class.oid */
  	map-new_relfilenode = new_rel-relfilenode;
  
- 	if (strcmp(old_rel-nspname, new_rel-nspname) != 0 ||
- 	strcmp(old_rel-relname, new_rel-relname) != 0)
- 		pg_log(PG_FATAL, mismatch of relation id: database \%s\, old rel %s.%s, new rel %s.%s\n,
- 			old_db-db_name, old_rel-nspname, old_rel-relname,
- 			new_rel-nspname, new_rel-relname);
- 
  	/* used only for logging and error reporing, old/new are identical */
  	snprintf(map-nspname, sizeof(map-nspname), %s, old_rel-nspname);
  	snprintf(map-relname, sizeof(map-relname), %s, old_rel-relname);
--- 113,118 

-- 
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] Fix for pg_upgrade migrating pg_largeobject_metadata

2011-01-07 Thread Bruce Momjian

Patch applied.

I did not backpatch to 9.0 because you can't migrate from 9.0 to 9.0
with the same catversion (because of tablespace conflict), and a pre-9.0
migration to 9.0 has not large object permissions to migrate.  In
summary, it didn't seem worth the risk, and was hard to test.

---

Bruce Momjian wrote:
 Bruce Momjian wrote:
  Tom Lane wrote:
   Bruce Momjian br...@momjian.us writes:
Tom Lane wrote:
That isn't going to work.  At least not unless you start trying to 
force
roles to have the same OIDs in the new installation.
   
If so I can use the CREATE ROLE ... SYSID clause when doing a binary
upgrade.
   
   Oh, I had forgotten we still had that wart in the grammar.
   It doesn't actually work:
   
 else if (strcmp(defel-defname, sysid) == 0)
 {
 ereport(NOTICE,
 (errmsg(SYSID can no longer be 
   specified)));
 }
   
   Not sure if it's better to try to make that work again than to add
   another hack in pg_upgrade_support.  On the whole that's a keyword
   I'd rather see us drop someday soon.
  
  OK, let me work on adding it to pg_upgrade_support.  Glad you saw this.
 
 I have fixed the bug by using pg_upgrade_support.  It was a little
 complicated because you need to install the pg_upgrade_support functions
 in the super-user database so it is available when you create the users
 in the first step of restoring the pg_dumpall file.
 
 I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
 upgrades.  It does not apply when coming from pre-9.0 because there was
 no pg_largeobject_metadata.
 
 For testing I did this:
 
   CREATE DATABASE lo;
   \c lo
   SELECT lo_import('/etc/motd');
   \set loid `psql -qt -c 'select loid from pg_largeobject' lo`
   CREATE ROLE user1;
   CREATE ROLE user2;
   -- force user2 to have a different user id on restore
   DROP ROLE user1;
   GRANT ALL ON LARGE OBJECT :loid TO user2;
 
 The fixed version shows:
 
   lo= select * from pg_largeobject_metadata;
lomowner |  lomacl
   --+--
  10 | {postgres=rw/postgres,user2=rw/postgres}
   (1 row)
 
 In the broken version, 'user2' was a raw oid, obviously wrong.
 
 Fortunately this was found during my testing and not reported as a bug
 by a pg_upgrade user.

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

  + It's impossible for everything to be true. +

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


[HACKERS] Fix for pg_upgrade migrating pg_largeobject_metadata

2011-01-05 Thread Bruce Momjian
Bruce Momjian wrote:
 Tom Lane wrote:
  Bruce Momjian br...@momjian.us writes:
   Tom Lane wrote:
   That isn't going to work.  At least not unless you start trying to force
   roles to have the same OIDs in the new installation.
  
   If so I can use the CREATE ROLE ... SYSID clause when doing a binary
   upgrade.
  
  Oh, I had forgotten we still had that wart in the grammar.
  It doesn't actually work:
  
  else if (strcmp(defel-defname, sysid) == 0)
  {
  ereport(NOTICE,
  (errmsg(SYSID can no longer be 
  specified)));
  }
  
  Not sure if it's better to try to make that work again than to add
  another hack in pg_upgrade_support.  On the whole that's a keyword
  I'd rather see us drop someday soon.
 
 OK, let me work on adding it to pg_upgrade_support.  Glad you saw this.

I have fixed the bug by using pg_upgrade_support.  It was a little
complicated because you need to install the pg_upgrade_support functions
in the super-user database so it is available when you create the users
in the first step of restoring the pg_dumpall file.

I am afraid we have to batckpatch this to fix to 9.0 for 9.0 to 9.0
upgrades.  It does not apply when coming from pre-9.0 because there was
no pg_largeobject_metadata.

For testing I did this:

CREATE DATABASE lo;
\c lo
SELECT lo_import('/etc/motd');
\set loid `psql -qt -c 'select loid from pg_largeobject' lo`
CREATE ROLE user1;
CREATE ROLE user2;
-- force user2 to have a different user id on restore
DROP ROLE user1;
GRANT ALL ON LARGE OBJECT :loid TO user2;

The fixed version shows:

lo= select * from pg_largeobject_metadata;
 lomowner |  lomacl
--+--
   10 | {postgres=rw/postgres,user2=rw/postgres}
(1 row)

In the broken version, 'user2' was a raw oid, obviously wrong.

Fortunately this was found during my testing and not reported as a bug
by a pg_upgrade user.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
index 52ab481..aba95f4 100644
*** /tmp/pgdiff.20347/YPWaqb_dump.c Wed Jan  5 20:52:07 2011
--- contrib/pg_upgrade/dump.c   Wed Jan  5 20:43:29 2011
*** generate_old_dump(void)
*** 33,39 
   *
   *This function splits pg_dumpall output into global values and
   *database creation, and per-db schemas.  This allows us to create
!  *the toast place holders between restoring these two parts of the
   *dump.  We split on the first \connect  after a CREATE ROLE
   *username match;  this is where the per-db restore starts.
   *
--- 33,39 
   *
   *This function splits pg_dumpall output into global values and
   *database creation, and per-db schemas.  This allows us to create
!  *the support functions between restoring these two parts of the
   *dump.  We split on the first \connect  after a CREATE ROLE
   *username match;  this is where the per-db restore starts.
   *
diff --git a/contrib/pg_upgrade/function.c b/contrib/pg_upgrade/function.c
index 2ab8e4f..5675551 100644
*** /tmp/pgdiff.20347/QpVBHa_function.c Wed Jan  5 20:52:07 2011
--- contrib/pg_upgrade/function.c   Wed Jan  5 20:26:33 2011
***
*** 13,35 
  
  
  /*
!  * install_support_functions()
   *
   * pg_upgrade requires some support functions that enable it to modify
   * backend behavior.
   */
  void
! install_support_functions(void)
  {
!   int dbnum;
! 
!   prep_status(Adding support functions to new cluster);
! 
!   for (dbnum = 0; dbnum  new_cluster.dbarr.ndbs; dbnum++)
!   {
!   DbInfo *new_db = new_cluster.dbarr.dbs[dbnum];
!   PGconn *conn = connectToServer(new_cluster, 
new_db-db_name);
! 
/* suppress NOTICE of dropped objects */
PQclear(executeQueryOrDie(conn,
  SET 
client_min_messages = warning;));
--- 13,28 
  
  
  /*
!  * install_db_support_functions()
   *
   * pg_upgrade requires some support functions that enable it to modify
   * backend behavior.
   */
  void
! install_db_support_functions(const char *db_name)
  {
!   PGconn *conn = connectToServer(new_cluster, db_name);
!   
/* suppress NOTICE of dropped objects */
PQclear(executeQueryOrDie(conn,
  SET 
client_min_messages = warning;));
*** install_support_functions(void)
*** 83,91 
  RETURNS VOID