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();
  }