Re: Fix for pg_upgrade and invalid indexes - Mailing list pgsql-hackers

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. +