Ignore invalid indexes in pg_dump
Hi,
If failures happen with CREATE INDEX CONCURRENTLY, the system will be let
with invalid indexes. I don't think that the user would like to see invalid
indexes of
an existing system being recreated as valid after a restore.
So why not removing from a dump invalid indexes with something like the
patch
attached?
This should perhaps be applied in pg_dump for versions down to 8.2 where
CREATE
INDEX CONCURRENTLY has been implemented?
I noticed some recent discussions about that:
/messages/by-id/20121207141236.GB4699@alvh.no-ip.org
In this case the problem has been fixed in pg_upgrade directly.
--
Michael
Attachments:
20130317_dump_only_valid_index.patchapplication/octet-stream; name=20130317_dump_only_valid_index.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 093be9e..0559e98 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4804,6 +4804,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"i.indexrelid = c.conindid AND "
"c.contype IN ('p','u','x')) "
"WHERE i.indrelid = '%u'::pg_catalog.oid "
+ "AND i.indisvalid "
"ORDER BY indexname",
tbinfo->dobj.catId.oid);
}
On 20 March 2013 02:51, Michael Paquier <michael.paquier@gmail.com> wrote:
If failures happen with CREATE INDEX CONCURRENTLY, the system will be let
with invalid indexes. I don't think that the user would like to see invalid
indexes of
an existing system being recreated as valid after a restore.
So why not removing from a dump invalid indexes with something like the
patch
attached?
This should perhaps be applied in pg_dump for versions down to 8.2 where
CREATE
INDEX CONCURRENTLY has been implemented?
Invalid also means currently-in-progress, so it would be better to keep them in.
I noticed some recent discussions about that:
/messages/by-id/20121207141236.GB4699@alvh.no-ip.org
In this case the problem has been fixed in pg_upgrade directly.
That is valid because the index build is clearly not in progress.
--
Simon Riggs 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
On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 20 March 2013 02:51, Michael Paquier <michael.paquier@gmail.com> wrote:
If failures happen with CREATE INDEX CONCURRENTLY, the system will be let
with invalid indexes. I don't think that the user would like to see invalid
indexes of
an existing system being recreated as valid after a restore.
So why not removing from a dump invalid indexes with something like the
patch
attached?
This should perhaps be applied in pg_dump for versions down to 8.2 where
CREATE
INDEX CONCURRENTLY has been implemented?Invalid also means currently-in-progress, so it would be better to keep them in.
For invalid indexes which are left hanging around in the database, if
the index definition is included by pg_dump, it will likely cause pain
during the restore. If the index build failed the first time and
hasn't been manually dropped and recreated since then, it's a good bet
it will fail the next time. Errors during restore can be more than
just a nuisance; consider restores with --single-transaction.
And if the index is simply currently-in-progress, it seems like the
expected behavior would be for pg_dump to ignore it anyway. We don't
include other DDL objects which are not yet committed while pg_dump is
running.
Josh
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Josh Kupershmidt <schmiddy@gmail.com> writes:
On Wed, Mar 20, 2013 at 2:00 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Invalid also means currently-in-progress, so it would be better to keep them in.
For invalid indexes which are left hanging around in the database, if
the index definition is included by pg_dump, it will likely cause pain
during the restore. If the index build failed the first time and
hasn't been manually dropped and recreated since then, it's a good bet
it will fail the next time. Errors during restore can be more than
just a nuisance; consider restores with --single-transaction.
And if the index is simply currently-in-progress, it seems like the
expected behavior would be for pg_dump to ignore it anyway. We don't
include other DDL objects which are not yet committed while pg_dump is
running.
I had been on the fence about what to do here, but I find Josh's
arguments persuasive, particularly the second one. Why shouldn't we
consider an in-progress index to be an uncommitted DDL change?
(Now admittedly, there won't *be* any uncommitted ordinary DDL on tables
while pg_dump is running, because it takes AccessShareLock on all
tables. But there could easily be uncommitted DDL against other types
of database objects, which pg_dump won't even see.)
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
On Thu, Mar 21, 2013 at 12:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I had been on the fence about what to do here, but I find Josh's
arguments persuasive, particularly the second one. Why shouldn't we
consider an in-progress index to be an uncommitted DDL change?(Now admittedly, there won't *be* any uncommitted ordinary DDL on tables
while pg_dump is running, because it takes AccessShareLock on all
tables. But there could easily be uncommitted DDL against other types
of database objects, which pg_dump won't even see.)
+1. Playing it safe is a better thing to do for sure, especially if a
restore would
fail. I didn't think about that first...
On top of checking indisvalid, I think that some additional checks on
indislive
and indisready are also necessary. As indisready has been introduced in 8.3
and
indislive has been added in 9.3, the attached patch is good I think.
I also added a note in the documentation about invalid indexes not being
dumped.
Perhaps this patch should be backpatched to previous versions in order to
have
the same consistent behavior.
Regards,
--
Michael
Attachments:
20130321_no_dump_indisvalid.patchapplication/octet-stream; name=20130321_no_dump_indisvalid.patchDownload
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 6d0f214..5267532 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1045,6 +1045,13 @@ CREATE DATABASE foo WITH TEMPLATE template0;
into an older server may require manual editing of the dump file
to remove syntax not understood by the older server.
</para>
+
+ <para>
+ Invalid indexes are not dumped as they might be in such a state due
+ due a <command>CREATE INDEX CONCURRENTLY</command> that failed
+ previously. This prevents the case where a restore could because of
+ the same index creation failure happening repetitively.
+ </para>
</refsect1>
<refsect1 id="pg-dump-examples">
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 093be9e..431a7ff 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4782,7 +4782,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
* is not.
*/
resetPQExpBuffer(query);
- if (fout->remoteVersion >= 90000)
+ if (fout->remoteVersion >= 90300)
{
appendPQExpBuffer(query,
"SELECT t.tableoid, t.oid, "
@@ -4804,6 +4804,66 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"i.indexrelid = c.conindid AND "
"c.contype IN ('p','u','x')) "
"WHERE i.indrelid = '%u'::pg_catalog.oid "
+ "AND i.indisvalid = TRUE "
+ "AND i.indisready = TRUE "
+ "AND i.indislive = TRUE "
+ "ORDER BY indexname",
+ tbinfo->dobj.catId.oid);
+ }
+ else if (fout->remoteVersion >= 90000)
+ {
+ appendPQExpBuffer(query,
+ "SELECT t.tableoid, t.oid, "
+ "t.relname AS indexname, "
+ "pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef, "
+ "t.relnatts AS indnkeys, "
+ "i.indkey, i.indisclustered, "
+ "c.contype, c.conname, "
+ "c.condeferrable, c.condeferred, "
+ "c.tableoid AS contableoid, "
+ "c.oid AS conoid, "
+ "pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
+ "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
+ "array_to_string(t.reloptions, ', ') AS options "
+ "FROM pg_catalog.pg_index i "
+ "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
+ "LEFT JOIN pg_catalog.pg_constraint c "
+ "ON (i.indrelid = c.conrelid AND "
+ "i.indexrelid = c.conindid AND "
+ "c.contype IN ('p','u','x')) "
+ "WHERE i.indrelid = '%u'::pg_catalog.oid "
+ "AND i.indisvalid = TRUE "
+ "AND i.indisready = TRUE "
+ "ORDER BY indexname",
+ tbinfo->dobj.catId.oid);
+ }
+ else if (fout->remoteVersion >= 80300)
+ {
+ appendPQExpBuffer(query,
+ "SELECT t.tableoid, t.oid, "
+ "t.relname AS indexname, "
+ "pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef, "
+ "t.relnatts AS indnkeys, "
+ "i.indkey, i.indisclustered, "
+ "c.contype, c.conname, "
+ "c.condeferrable, c.condeferred, "
+ "c.tableoid AS contableoid, "
+ "c.oid AS conoid, "
+ "null AS condef, "
+ "(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
+ "array_to_string(t.reloptions, ', ') AS options "
+ "FROM pg_catalog.pg_index i "
+ "JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
+ "LEFT JOIN pg_catalog.pg_depend d "
+ "ON (d.classid = t.tableoid "
+ "AND d.objid = t.oid "
+ "AND d.deptype = 'i') "
+ "LEFT JOIN pg_catalog.pg_constraint c "
+ "ON (d.refclassid = c.tableoid "
+ "AND d.refobjid = c.oid) "
+ "WHERE i.indrelid = '%u'::pg_catalog.oid "
+ "AND i.indisvalid = TRUE "
+ "AND i.indisready = TRUE "
"ORDER BY indexname",
tbinfo->dobj.catId.oid);
}
@@ -4832,6 +4892,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"ON (d.refclassid = c.tableoid "
"AND d.refobjid = c.oid) "
"WHERE i.indrelid = '%u'::pg_catalog.oid "
+ "AND i.indisvalid = TRUE "
"ORDER BY indexname",
tbinfo->dobj.catId.oid);
}
Michael Paquier <michael.paquier@gmail.com> writes:
On top of checking indisvalid, I think that some additional checks on
indislive and indisready are also necessary.
Those are not necessary, as an index that is marked indisvalid should
certainly also have those flags set. If it didn't require making two
new version distinctions in getIndexes(), I'd be okay with the extra
checks; but as-is I think the maintenance pain this would add greatly
outweighs any likely value.
I've committed this in the simpler form that just adds indisvalid
checks to the appropriate version cases.
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
On Wed, Mar 27, 2013 at 6:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On top of checking indisvalid, I think that some additional checks on
indislive and indisready are also necessary.Those are not necessary, as an index that is marked indisvalid should
certainly also have those flags set. If it didn't require making two
new version distinctions in getIndexes(), I'd be okay with the extra
checks; but as-is I think the maintenance pain this would add greatly
outweighs any likely value.I've committed this in the simpler form that just adds indisvalid
checks to the appropriate version cases.
Thanks.
--
Michael