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

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

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

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.

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

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

"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

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

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?

Andres

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

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

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

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

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

"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 or partially 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

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

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

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

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

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

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

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

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

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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. +

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