[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-hackers
Subject:    Re: [HACKERS] Fix for pg_upgrade and invalid indexes
From:       Bruce Momjian <bruce () momjian ! us>
Date:       2013-03-31 2:21:41
Message-ID: 20130331022141.GA29666 () momjian ! us
[Download RAW message or body]


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 <andres@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  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + It's impossible for everything to be true. +

> diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
> 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 ----
> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> new file mode 100644
> index a5aa40f..c5c3698
> *** a/contrib/pg_upgrade/info.c
> --- b/contrib/pg_upgrade/info.c
> *************** get_rel_infos(ClusterInfo *cluster, DbIn
> *** 282,288 ****
> --- 282,294 ----
>   			 "CREATE TEMPORARY TABLE info_rels (reloid) AS SELECT c.oid "
>   			 "FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n "
>   			 "	   ON c.relnamespace = n.oid "
> + 			 "LEFT 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 "

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


-- 
  Bruce Momjian  <bruce@momjian.us>        http://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
[prev in list] [next in list] [prev in thread] [next in thread]