Re: Fix for pg_upgrade and invalid indexes

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(at)2ndquadrant(dot)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(at)momjian(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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