Thread: Re: Ignore invalid indexes in pg_dump.
On Tue, Mar 26, 2013 at 09:43:54PM +0000, Tom Lane wrote: > Ignore invalid indexes in pg_dump. > > Dumping invalid indexes can cause problems at restore time, for example > if the reason the index creation failed was because it tried to enforce > a uniqueness condition not satisfied by the table's data. Also, if the > index creation is in fact still in progress, it seems reasonable to > consider it to be an uncommitted DDL change, which pg_dump wouldn't be > expected to dump anyway. > > Back-patch to all active versions, and teach them to ignore invalid > indexes in servers back to 8.2, where the concept was introduced. This commit affects pg_upgrade. You might remember we had to patch pg_upgrade to prevent it from migrating clusters with invalid indexes in December, 2012: http://momjian.us/main/blogs/pgblog/2012.html#December_14_2012 This was released on February 7, 2013 in 9.2.3 and other back branches: http://www.postgresql.org/docs/9.2/static/release-9-2-3.html This git commit means that pg_upgrade can again migrate systems with invalid indexes as pg_upgrade can just skip migrating them because pg_dump will dump the SQL commands to create them --- previously pg_upgrade threw an error. 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"? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@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.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> schrieb: >Bruce Momjian <bruce@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. You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On Thu, Mar 28, 2013 at 10:31:51PM +0100, anarazel@anarazel.de wrote: > > > Tom Lane <tgl@sss.pgh.pa.us> schrieb: > > >Bruce Momjian <bruce@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. > > You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? Well, if it is dropped, pg_dump will not dump it. At this point though, pg_upgrade is either running in check mode, or it is the only user. I think we are OK. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
"anarazel@anarazel.de" <andres@anarazel.de> writes:
> Tom Lane <tgl@sss.pgh.pa.us> schrieb:
>> 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.
> You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No?
No, he doesn't need to look at indisready/indislive; if either of those
flags are off then indisvalid should certainly be off too. (If it
isn't, queries against the table are already in trouble.)
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> schrieb: >"anarazel@anarazel.de" <andres@anarazel.de> writes: >> Tom Lane <tgl@sss.pgh.pa.us> schrieb: >>> 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. > >> You need to look at inisready in 9.2 since thats used for about to be >dropped indexes. No? > >No, he doesn't need to look at indisready/indislive; if either of those >flags are off then indisvalid should certainly be off too. (If it >isn't, queries against the table are already in trouble.) 9.2 represents inisdead as live && !ready, doesn't it? So just looking at indislive will include about to be dropped or partiallydropped indexes? Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On Thu, Mar 28, 2013 at 10:47:55PM +0100, anarazel@anarazel.de wrote: > > > Tom Lane <tgl@sss.pgh.pa.us> schrieb: > > >"anarazel@anarazel.de" <andres@anarazel.de> writes: > >> Tom Lane <tgl@sss.pgh.pa.us> schrieb: > >>> 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. > > > >> You need to look at inisready in 9.2 since thats used for about to > >> be > >dropped indexes. No? > > > >No, he doesn't need to look at indisready/indislive; if either of > >those flags are off then indisvalid should certainly be off too. (If > >it isn't, queries against the table are already in trouble.) > > 9.2 represents inisdead as live && !ready, doesn't it? So just looking > at indislive will include about to be dropped or partially dropped > indexes? Where do you see 'inisdead' defined? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On 2013-03-28 17:54:08 -0400, Bruce Momjian wrote: > On Thu, Mar 28, 2013 at 10:47:55PM +0100, anarazel@anarazel.de wrote: > > > > > > Tom Lane <tgl@sss.pgh.pa.us> schrieb: > > > > >"anarazel@anarazel.de" <andres@anarazel.de> writes: > > >> Tom Lane <tgl@sss.pgh.pa.us> schrieb: > > >>> 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. > > > > > >> You need to look at inisready in 9.2 since thats used for about to > > >> be > > >dropped indexes. No? > > > > > >No, he doesn't need to look at indisready/indislive; if either of > > >those flags are off then indisvalid should certainly be off too. (If > > >it isn't, queries against the table are already in trouble.) > > > > 9.2 represents inisdead as live && !ready, doesn't it? So just looking > > at indislive will include about to be dropped or partially dropped > > indexes? > > Where do you see 'inisdead' defined? Sorry, its named the reverse, indislive. And its only there in 9.3+... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
"anarazel@anarazel.de" <andres@anarazel.de> writes:
> 9.2 represents inisdead as live && !ready, doesn't it? So just looking at indislive will include about to be dropped
orpartially dropped indexes?
Ooooh ... you're right, in 9.2 (only) we need to check both indisvalid
and indisready (cf IndexIsValid() macro in the different branches).
So that's actually a bug in the committed pg_dump patch, too. I'll fix
that shortly.
regards, tom lane
On 2013-03-28 17:35:08 -0400, Bruce Momjian wrote: > On Thu, Mar 28, 2013 at 10:31:51PM +0100, anarazel@anarazel.de wrote: > > > > > > Tom Lane <tgl@sss.pgh.pa.us> schrieb: > > > > >Bruce Momjian <bruce@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. > > > > You need to look at inisready in 9.2 since thats used for about to be dropped indexes. No? > > Well, if it is dropped, pg_dump will not dump it. At this point though, > pg_upgrade is either running in check mode, or it is the only user. I > think we are OK. Its about DROP INDEX CONCURRENTLY which can leave indexes in a partial state visible to the outside. Either just transiently while a DROP CONCURRENLTY is going on or even permanently if the server crashed during such a drop or similar. c.f. index.c:index_drop Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Mar 28, 2013 at 05:27:28PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Attachment
Bruce Momjian <bruce@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
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 <bruce@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
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.)
regards, tom lane
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. +
Attachment
On 2013-03-29 19:03:05 -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.) 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
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. +