reindex concurrently and two toast indexes

From 990d265e5d576b3b4133232f302d6207987f1511 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.

Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands.  As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
 src/backend/catalog/index.c                   | 29 +++++++++++
 .../expected/reindex-concurrently.out         | 49 ++++++++++++++++++-
 .../isolation/specs/reindex-concurrently.spec | 23 +++++++++
 3 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
 #include "catalog/pg_depend.h"
 #include "catalog/pg_description.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
 
+			/*
+			 * We skip any invalid index on a TOAST table.  Those can only be
+			 * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+			 * rebuild it it won't be possible to drop it anymore.
+			 */
+			if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+			{
+				HeapTuple	tup;
+				bool		skipit;
+
+				tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+				if (!HeapTupleIsValid(tup))
+					elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+				skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+				ReleaseSysCache(tup);
+
+				if (skipit)
+				{
+					ereport(NOTICE,
+							 (errmsg("skipping invalid index \"%s.%s\"",
+									get_namespace_name(get_rel_namespace(indexOid)),
+									get_rel_name(indexOid))));
+					continue;
+				}
+			}
+
 			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
 						  persistence, options);
 
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..fa9039c125 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
 
 starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,50 @@ step end1: COMMIT;
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
 step end2: COMMIT;
 step reindex: <... completed>
+
+starting permutation: lock timeout reindex unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data           
+
+aa             
+step timeout: SET statement_timeout to 5000;
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step unlock: SELECT pg_sleep(6); COMMIT;
+pg_sleep       
+
+               
+step reindex: <... completed>
+error in steps unlock reindex: ERROR:  canceling statement due to statement timeout
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..0599e8e213 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
 
 session "s3"
 step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+step "timeout" { SET statement_timeout to 5000; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { SELECT pg_sleep(6); COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C"; }
 
 permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
 permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# TOAST table.  Any  following successful REINDEX should leave this index as
+# invalid, otherwise we would end up with a useless and duplicated index that
+# can't be dropped.
+permutation "lock" "timeout" "reindex" "unlock" "check_invalid"
+    "normal_reindex" "check_invalid"
+    "nowarn" "reindex" "check_invalid"
-- 
2.20.1

From 1b1bd50e4af4a2034638898129e6e49e3f4999da Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.

Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands.  As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
 src/backend/catalog/index.c                   | 29 ++++++++++
 .../expected/reindex-concurrently.out         | 55 ++++++++++++++++++-
 .../isolation/specs/reindex-concurrently.spec | 27 +++++++++
 3 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
 #include "catalog/pg_depend.h"
 #include "catalog/pg_description.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
 
+			/*
+			 * We skip any invalid index on a TOAST table.  Those can only be
+			 * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+			 * rebuild it it won't be possible to drop it anymore.
+			 */
+			if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+			{
+				HeapTuple	tup;
+				bool		skipit;
+
+				tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+				if (!HeapTupleIsValid(tup))
+					elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+				skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+				ReleaseSysCache(tup);
+
+				if (skipit)
+				{
+					ereport(NOTICE,
+							 (errmsg("skipping invalid index \"%s.%s\"",
+									get_namespace_name(get_rel_namespace(indexOid)),
+									get_rel_name(indexOid))));
+					continue;
+				}
+			}
+
 			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
 						  persistence, options);
 
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..012b4874dd 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
 
 starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,56 @@ step end1: COMMIT;
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
 step end2: COMMIT;
 step reindex: <... completed>
+
+starting permutation: lock reindex sleep kill unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data           
+
+aa             
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step sleep: SELECT pg_sleep(1);
+pg_sleep       
+
+               
+step kill: SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+    WHERE application_name = 's3_reindex_concurrently'
+    AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;'
+pg_cancel_backend
+
+t              
+step reindex: <... completed>
+error in steps kill reindex: ERROR:  canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..ecd784cc4a 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -30,7 +30,27 @@ step "del2" { DELETE FROM reind_con_tab WHERE data = 'cccc'; }
 step "end2" { COMMIT; }
 
 session "s3"
+setup { SET application_name TO "s3_reindex_concurrently"; }
 step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "sleep" { SELECT pg_sleep(1); }
+step "kill" { SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+    WHERE application_name = 's3_reindex_concurrently'
+    AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;' }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C"; }
 
 permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
 permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +58,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# TOAST table.  Any  following successful REINDEX should leave this index as
+# invalid, otherwise we would end up with a useless and duplicated index that
+# can't be dropped.
+permutation "lock" "reindex" "sleep" "kill" "unlock" "check_invalid"
+    "normal_reindex" "check_invalid"
+    "nowarn" "reindex" "check_invalid"
-- 
2.20.1

diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 0cd6fcf027..5811d082b0 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -200,10 +200,10 @@ extern long changeDependencyFor(Oid classId, Oid objectId,
 								Oid refClassId, Oid oldRefObjectId,
 								Oid newRefObjectId);
 
-extern long changeDependenciesOf(Oid classId, Oid oldObjectId,
+extern long switchDependenciesOf(Oid classId, Oid oldObjectId,
 								 Oid newObjectId);
 
-extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+extern long switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
 								 Oid newRefObjectId);
 
 extern Oid	getExtensionOfObject(Oid classId, Oid objectId);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..fb9bb276bb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,14 +1675,10 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	}
 
 	/*
-	 * Move all dependencies of and on the old index to the new one.  First
-	 * remove any dependencies that the new index may have to provide an
-	 * initial clean state for the dependency switch, and then move all the
-	 * dependencies from the old index to the new one.
+	 * Switch all dependencies of and on the old and new indexes.
 	 */
-	deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
-	changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
-	changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+	switchDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
+	switchDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
 
 	/*
 	 * Copy over statistics from old to new index
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f9af245eec..11caa38083 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -396,7 +396,7 @@ changeDependencyFor(Oid classId, Oid objectId,
 }
 
 /*
- * Adjust all dependency records to come from a different object of the same type
+ * Switch dependency records between two objects of the same type
  *
  * classId/oldObjectId specify the old referencing object.
  * newObjectId is the new referencing object (must be of class classId).
@@ -404,38 +404,41 @@ changeDependencyFor(Oid classId, Oid objectId,
  * Returns the number of records updated.
  */
 long
-changeDependenciesOf(Oid classId, Oid oldObjectId,
+switchDependenciesOf(Oid classId, Oid oldObjectId,
 					 Oid newObjectId)
 {
 	long		count = 0;
 	Relation	depRel;
-	ScanKeyData key[2];
+	ScanKeyData key;
 	SysScanDesc scan;
 	HeapTuple	tup;
 
 	depRel = table_open(DependRelationId, RowExclusiveLock);
 
-	ScanKeyInit(&key[0],
+	ScanKeyInit(&key,
 				Anum_pg_depend_classid,
 				BTEqualStrategyNumber, F_OIDEQ,
 				ObjectIdGetDatum(classId));
-	ScanKeyInit(&key[1],
-				Anum_pg_depend_objid,
-				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(oldObjectId));
 
 	scan = systable_beginscan(depRel, DependDependerIndexId, true,
-							  NULL, 2, key);
+							  NULL, 1, &key);
 
 	while (HeapTupleIsValid((tup = systable_getnext(scan))))
 	{
 		Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
 
+		if (depform->objid != oldObjectId &&
+			depform->objid != newObjectId)
+			continue;
+
 		/* make a modifiable copy */
 		tup = heap_copytuple(tup);
 		depform = (Form_pg_depend) GETSTRUCT(tup);
 
-		depform->objid = newObjectId;
+		if (depform->objid == oldObjectId)
+			depform->objid = newObjectId;
+		else
+			depform->objid = oldObjectId;
 
 		CatalogTupleUpdate(depRel, &tup->t_self, tup);
 
@@ -452,7 +455,7 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
 }
 
 /*
- * Adjust all dependency records to point to a different object of the same type
+ * Switch all dependency records between two objects of the same type.
  *
  * refClassId/oldRefObjectId specify the old referenced object.
  * newRefObjectId is the new referenced object (must be of class refClassId).
@@ -460,74 +463,75 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
  * Returns the number of records updated.
  */
 long
-changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
 					 Oid newRefObjectId)
 {
 	long		count = 0;
 	Relation	depRel;
-	ScanKeyData key[2];
+	ScanKeyData key;
 	SysScanDesc scan;
 	HeapTuple	tup;
-	ObjectAddress objAddr;
-	bool		newIsPinned;
+	ObjectAddress newObjAddr;
+	ObjectAddress oldObjAddr;
 
 	depRel = table_open(DependRelationId, RowExclusiveLock);
 
 	/*
-	 * If oldRefObjectId is pinned, there won't be any dependency entries on
-	 * it --- we can't cope in that case.  (This isn't really worth expending
-	 * code to fix, in current usage; it just means you can't rename stuff out
-	 * of pg_catalog, which would likely be a bad move anyway.)
+	 * If oldRefObjectId or newRefObjectId are pinned, there won't be any
+	 * dependency entries on it --- we can't cope in that case.  (This
+	 * isn't really worth expending code to fix, in current usage; it
+	 * just means you can't rename stuff out of pg_catalog, which would
+	 * likely be a bad move anyway.)
 	 */
-	objAddr.classId = refClassId;
-	objAddr.objectId = oldRefObjectId;
-	objAddr.objectSubId = 0;
+	oldObjAddr.classId = refClassId;
+	oldObjAddr.objectId = oldRefObjectId;
+	oldObjAddr.objectSubId = 0;
 
-	if (isObjectPinned(&objAddr, depRel))
+	if (isObjectPinned(&oldObjAddr, depRel))
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot remove dependency on %s because it is a system object",
-						getObjectDescription(&objAddr))));
+						getObjectDescription(&oldObjAddr))));
 
-	/*
-	 * We can handle adding a dependency on something pinned, though, since
-	 * that just means deleting the dependency entry.
-	 */
-	objAddr.objectId = newRefObjectId;
+	newObjAddr.classId = refClassId;
+	newObjAddr.objectId = newRefObjectId;
+	newObjAddr.objectSubId = 0;
 
-	newIsPinned = isObjectPinned(&objAddr, depRel);
+	if (isObjectPinned(&newObjAddr, depRel))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot remove dependency on %s because it is a system object",
+						getObjectDescription(&newObjAddr))));
 
 	/* Now search for dependency records */
-	ScanKeyInit(&key[0],
+	ScanKeyInit(&key,
 				Anum_pg_depend_refclassid,
 				BTEqualStrategyNumber, F_OIDEQ,
 				ObjectIdGetDatum(refClassId));
-	ScanKeyInit(&key[1],
-				Anum_pg_depend_refobjid,
-				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(oldRefObjectId));
 
 	scan = systable_beginscan(depRel, DependReferenceIndexId, true,
-							  NULL, 2, key);
+							  NULL, 1, &key);
 
 	while (HeapTupleIsValid((tup = systable_getnext(scan))))
 	{
 		Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
 
-		if (newIsPinned)
-			CatalogTupleDelete(depRel, &tup->t_self);
-		else
-		{
-			/* make a modifiable copy */
-			tup = heap_copytuple(tup);
-			depform = (Form_pg_depend) GETSTRUCT(tup);
+		if (depform->refobjid != oldRefObjectId &&
+			depform->refobjid != newRefObjectId)
+			continue;
 
+		/* make a modifiable copy */
+		tup = heap_copytuple(tup);
+		depform = (Form_pg_depend) GETSTRUCT(tup);
+
+		if (depform->refobjid == oldRefObjectId)
 			depform->refobjid = newRefObjectId;
+		else
+			depform->refobjid = oldRefObjectId;
 
-			CatalogTupleUpdate(depRel, &tup->t_self, tup);
+		CatalogTupleUpdate(depRel, &tup->t_self, tup);
 
-			heap_freetuple(tup);
-		}
+		heap_freetuple(tup);
 
 		count++;
 	}

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..736bc9f66c 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,14 +1675,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	}
 
 	/*
-	 * Move all dependencies of and on the old index to the new one.  First
-	 * remove any dependencies that the new index may have to provide an
-	 * initial clean state for the dependency switch, and then move all the
-	 * dependencies from the old index to the new one.
+	 * Swap all dependencies of and on the old index to the new one, and
+	 * vice-versa.
 	 */
-	deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
 	changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
 	changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+	changeDependenciesOf(RelationRelationId, newIndexId, oldIndexId);
+	changeDependenciesOn(RelationRelationId, newIndexId, oldIndexId);
 
 	/*
 	 * Copy over statistics from old to new index

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..7223679033 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,12 +1675,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	}
 
 	/*
-	 * Move all dependencies of and on the old index to the new one.  First
-	 * remove any dependencies that the new index may have to provide an
-	 * initial clean state for the dependency switch, and then move all the
-	 * dependencies from the old index to the new one.
+	 * Swap all dependencies of and on the old index to the new one, and
+	 * vice-versa.  Note that a call to CommandCounterIncrement() would cause
+	 * duplicate entries in pg_depend, so this should not be done.
 	 */
-	deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
+	changeDependenciesOf(RelationRelationId, newIndexId, oldIndexId);
+	changeDependenciesOn(RelationRelationId, newIndexId, oldIndexId);
+
 	changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
 	changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
 

From 77ec865a9a655b2b973846f9a8fa93c966ca55f5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.

Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands.  As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
 src/backend/catalog/index.c      | 30 ++++++++++++++++++++
 src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++-------
 2 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
 #include "catalog/pg_depend.h"
 #include "catalog/pg_description.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
 
+			/*
+			 * We skip any invalid index on a TOAST table.  Those can only be
+			 * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+			 * rebuild it it won't be possible to drop it anymore.
+			 */
+			if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+			{
+				HeapTuple	tup;
+				bool		skipit;
+
+				tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+				if (!HeapTupleIsValid(tup))
+					elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+				skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+				ReleaseSysCache(tup);
+
+				if (skipit)
+				{
+					ereport(WARNING,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+									get_namespace_name(get_rel_namespace(indexOid)),
+									get_rel_name(indexOid))));
+					continue;
+				}
+			}
+
 			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
 						  persistence, options);
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ec20ba38d1..7985d98aa8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
 #include "catalog/pg_am.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_opfamily.h"
 #include "catalog/pg_tablespace.h"
@@ -2337,6 +2338,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	Oid			indOid;
 	Relation	irel;
 	char		persistence;
+	bool		invalidtoastindex;
 
 	/*
 	 * Find and lock index, and check permissions on table; use callback to
@@ -2362,6 +2364,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	 */
 	irel = index_open(indOid, NoLock);
 
+	invalidtoastindex = (irel->rd_rel->relkind == RELKIND_INDEX &&
+						 !irel->rd_index->indisvalid);
+
 	if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
 	{
 		ReindexPartitionedIndex(irel);
@@ -2371,6 +2376,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	persistence = irel->rd_rel->relpersistence;
 	index_close(irel, NoLock);
 
+	if (invalidtoastindex)
+	{
+		ereport(WARNING,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+						get_namespace_name(get_rel_namespace(indOid)),
+						get_rel_name(indOid))));
+		return;
+	}
+
 	if (concurrent && persistence != RELPERSISTENCE_TEMP)
 		ReindexRelationConcurrently(indOid, options);
 	else
@@ -2890,25 +2905,37 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		case RELKIND_INDEX:
 			{
 				Oid			heapId = IndexGetRelation(relationOid, false);
+				Relation	indexRelation = index_open(relationOid,
+													   ShareUpdateExclusiveLock);
 
 				if (IsCatalogRelationOid(heapId))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
+				else if (!indexRelation->rd_index->indisvalid)
+					ereport(WARNING,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
+									get_namespace_name(get_rel_namespace(relationOid)),
+									get_rel_name(relationOid))));
+				else
+				{
+					/* Save the list of relation OIDs in private context */
+					oldcontext = MemoryContextSwitchTo(private_context);
 
-				/* Save the list of relation OIDs in private context */
-				oldcontext = MemoryContextSwitchTo(private_context);
+					/* Track the heap relation of this index for session locks */
+					heapRelationIds = list_make1_oid(heapId);
 
-				/* Track the heap relation of this index for session locks */
-				heapRelationIds = list_make1_oid(heapId);
+					/*
+					 * Save the list of relation OIDs in private context.  Note
+					 * that invalid indexes are allowed here.
+					 */
+					indexIds = lappend_oid(indexIds, relationOid);
 
-				/*
-				 * Save the list of relation OIDs in private context.  Note
-				 * that invalid indexes are allowed here.
-				 */
-				indexIds = lappend_oid(indexIds, relationOid);
+					MemoryContextSwitchTo(oldcontext);
+				}
 
-				MemoryContextSwitchTo(oldcontext);
+				index_close(indexRelation, NoLock);
 				break;
 			}
 		case RELKIND_PARTITIONED_TABLE:
-- 
2.20.1

From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.

Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands.  As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
 src/backend/catalog/index.c      | 30 ++++++++++++++++++++++++++++++
 src/backend/commands/indexcmds.c | 15 +++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
 #include "catalog/pg_depend.h"
 #include "catalog/pg_description.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
 
+			/*
+			 * We skip any invalid index on a TOAST table.  Those can only be
+			 * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+			 * rebuild it it won't be possible to drop it anymore.
+			 */
+			if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+			{
+				HeapTuple	tup;
+				bool		skipit;
+
+				tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+				if (!HeapTupleIsValid(tup))
+					elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+				skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+				ReleaseSysCache(tup);
+
+				if (skipit)
+				{
+					ereport(WARNING,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+									get_namespace_name(get_rel_namespace(indexOid)),
+									get_rel_name(indexOid))));
+					continue;
+				}
+			}
+
 			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
 						  persistence, options);
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3f3a89fe92..08d74fecd1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
 #include "catalog/pg_am.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_opfamily.h"
 #include "catalog/pg_tablespace.h"
@@ -2309,6 +2310,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	Oid			indOid;
 	Relation	irel;
 	char		persistence;
+	bool		invalidtoastindex;
 
 	/*
 	 * Find and lock index, and check permissions on table; use callback to
@@ -2334,6 +2336,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	 */
 	irel = index_open(indOid, NoLock);
 
+	invalidtoastindex = (irel->rd_rel->relnamespace == PG_TOAST_NAMESPACE &&
+						 !irel->rd_index->indisvalid);
+
 	if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
 	{
 		ReindexPartitionedIndex(irel);
@@ -2343,6 +2348,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	persistence = irel->rd_rel->relpersistence;
 	index_close(irel, NoLock);
 
+	if (invalidtoastindex)
+	{
+		ereport(WARNING,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+						get_namespace_name(get_rel_namespace(indOid)),
+						get_rel_name(indOid))));
+		return;
+	}
+
 	if (concurrent && persistence != RELPERSISTENCE_TEMP)
 		ReindexRelationConcurrently(indOid, options);
 	else
-- 
2.20.1

From 8480fa105032e15ef76926f9247b2f5af97223d0 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:40:56 +0100
Subject: [PATCH 1/2] Add an optional timeout value to isolationtester steps.

Some sanity checks can require a command to wait on a lock and eventually be
cancelled.  The only way to do that was to rely on calls to pg_cancel_backend()
filtering pg_stat_activity view, but this isn't a satisfactory solution as
there's no way to guarantee that only the wanted backend will be canceled.

Instead, add a new optional "timeout val" clause to the step definition.  When
this clause is specified, isolationtester will actively wait on that command,
and issue a cancel when the given timeout is reached.

Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200305035354.GQ2593%40paquier.xyz
---
 src/test/isolation/README            | 11 ++++++--
 src/test/isolation/isolationtester.c | 42 ++++++++++++++++++++++++----
 src/test/isolation/isolationtester.h |  2 ++
 src/test/isolation/specparse.y       | 18 ++++++++++--
 src/test/isolation/specscanner.l     |  8 ++++++
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/src/test/isolation/README b/src/test/isolation/README
index 217953d183..827bea0c42 100644
--- a/src/test/isolation/README
+++ b/src/test/isolation/README
@@ -86,10 +86,12 @@ session "<name>"
 
   Each step has the syntax
 
-  step "<name>" { <SQL> }
+  step "<name>" { <SQL> } [ TIMEOUT seconds ]
 
-  where <name> is a name identifying this step, and SQL is a SQL statement
-  (or statements, separated by semicolons) that is executed in the step.
+  where <name> is a name identifying this step, SQL is a SQL statement
+  (or statements, separated by semicolons) that is executed in the step and
+  seconds an optional timeout for the given SQL statement(s) to wait on before
+  canceling it.
   Step names must be unique across the whole spec file.
 
 permutation "<step name>" ...
@@ -125,6 +127,9 @@ after PGISOLATIONTIMEOUT seconds.  If the cancel doesn't work, isolationtester
 will exit uncleanly after a total of twice PGISOLATIONTIMEOUT.  Testing
 invalid permutations should be avoided because they can make the isolation
 tests take a very long time to run, and they serve no useful testing purpose.
+If a test specified the option timeout specification, then isolationtester will
+actively wait for the step commands completion rather than continuing with the
+permutation next step, and send a cancel once the given timeout is reached.
 
 Note that isolationtester recognizes that a command has blocked by looking
 to see if it is shown as waiting in the pg_locks view; therefore, only
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f80261c022..dd5d335027 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -120,10 +120,28 @@ main(int argc, char **argv)
 	spec_yyparse();
 	testspec = &parseresult;
 
-	/* Create a lookup table of all steps. */
+	/*
+	 * Create a lookup table of all steps and validate any timeout
+	 * specification.
+	 */
 	nallsteps = 0;
 	for (i = 0; i < testspec->nsessions; i++)
+	{
 		nallsteps += testspec->sessions[i]->nsteps;
+		for (j = 0; j < testspec->sessions[i]->nsteps; j++)
+		{
+			if ((testspec->sessions[i]->steps[j]->timeout  * USECS_PER_SEC) >=
+					(max_step_wait /2))
+			{
+				fprintf(stderr, "step %s: step timeout (%d) should be less"
+						" than global timeout (%ld)",
+						testspec->sessions[i]->steps[j]->name,
+						testspec->sessions[i]->steps[j]->timeout,
+						(max_step_wait / USECS_PER_SEC));
+				exit(1);
+			}
+		}
+	}
 
 	allsteps = pg_malloc(nallsteps * sizeof(Step *));
 
@@ -587,8 +605,14 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
 			exit(1);
 		}
 
-		/* Try to complete this step without blocking.  */
-		mustwait = try_complete_step(testspec, step, STEP_NONBLOCK);
+		/*
+		 * Try to complete this step without blocking, unless the step has a
+		 * timeout.
+		 */
+		mustwait = try_complete_step(testspec, step,
+				(step->timeout == 0 ? STEP_NONBLOCK : 0));
+		if (step->timeout != 0)
+			report_error_message(step);
 
 		/* Check for completion of any steps that were previously waiting. */
 		w = 0;
@@ -721,6 +745,7 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 		{
 			struct timeval current_time;
 			int64		td;
+			int64		step_timeout;
 
 			/* If it's OK for the step to block, check whether it has. */
 			if (flags & STEP_NONBLOCK)
@@ -778,6 +803,11 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 			td *= USECS_PER_SEC;
 			td += (int64) current_time.tv_usec - (int64) start_time.tv_usec;
 
+			if (step->timeout)
+				step_timeout = step->timeout * USECS_PER_SEC;
+			else
+				step_timeout = max_step_wait;
+
 			/*
 			 * After max_step_wait microseconds, try to cancel the query.
 			 *
@@ -787,7 +817,7 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 			 * failing, but remaining permutations and tests should still be
 			 * OK.
 			 */
-			if (td > max_step_wait && !canceled)
+			if (td > step_timeout && !canceled)
 			{
 				PGcancel   *cancel = PQgetCancel(conn);
 
@@ -812,13 +842,13 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 			}
 
 			/*
-			 * After twice max_step_wait, just give up and die.
+			 * After twice the step timeout, just give up and die.
 			 *
 			 * Since cleanup steps won't be run in this case, this may cause
 			 * later tests to fail.  That stinks, but it's better than waiting
 			 * forever for the server to respond to the cancel.
 			 */
-			if (td > 2 * max_step_wait)
+			if (td > 2 * step_timeout)
 			{
 				fprintf(stderr, "step %s timed out after %d seconds\n",
 						step->name, (int) (td / USECS_PER_SEC));
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 9cf5012416..0a0dd54ff3 100644
--- a/src/test/isolation/isolationtester.h
+++ b/src/test/isolation/isolationtester.h
@@ -33,6 +33,8 @@ struct Step
 	char	   *name;
 	char	   *sql;
 	char	   *errormsg;
+	int			timeout;
+	struct timeval start_time;
 };
 
 typedef struct
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index 5e007e1bf0..76842c42ab 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -25,6 +25,7 @@ TestSpec		parseresult;			/* result of parsing is left here */
 %union
 {
 	char	   *str;
+	int			ival;
 	Session	   *session;
 	Step	   *step;
 	Permutation *permutation;
@@ -43,9 +44,11 @@ TestSpec		parseresult;			/* result of parsing is left here */
 %type <session> session
 %type <step> step
 %type <permutation> permutation
+%type <ival> opt_timeout
 
 %token <str> sqlblock string_literal
-%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST
+%token <ival> INTEGER
+%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST TIMEOUT
 
 %%
 
@@ -140,16 +143,27 @@ step_list:
 
 
 step:
-			STEP string_literal sqlblock
+			STEP string_literal sqlblock opt_timeout
 			{
 				$$ = pg_malloc(sizeof(Step));
 				$$->name = $2;
 				$$->sql = $3;
 				$$->used = false;
 				$$->errormsg = NULL;
+				$$->timeout = $4;
 			}
 		;
 
+opt_timeout:
+			TIMEOUT INTEGER
+			{
+				$$ = $2;
+			}
+			| /* EMPTY */
+			{
+				$$ = 0;
+			}
+		;
 
 opt_permutation_list:
 			permutation_list
diff --git a/src/test/isolation/specscanner.l b/src/test/isolation/specscanner.l
index 410f17727e..1ec1812569 100644
--- a/src/test/isolation/specscanner.l
+++ b/src/test/isolation/specscanner.l
@@ -53,6 +53,7 @@ session			{ return SESSION; }
 setup			{ return SETUP; }
 step			{ return STEP; }
 teardown		{ return TEARDOWN; }
+timeout			{ return TIMEOUT; }
 
 [\n]			{ yyline++; }
 {comment}		{ /* ignore */ }
@@ -96,6 +97,13 @@ teardown		{ return TEARDOWN; }
 					yyerror("unterminated sql block");
 				}
 
+ /* integer */
+[0-9]+			{
+					yylval.ival = atoi(yytext);
+					return INTEGER;
+
+				}
+
 .				{
 					fprintf(stderr, "syntax error at line %d: unexpected character \"%s\"\n", yyline, yytext);
 					exit(1);
-- 
2.25.1

From 1d112c1bbb563a76198435d60047e8a2b96bcd4a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:51:35 +0100
Subject: [PATCH 2/2] Add regression tests for failed REINDEX TABLE
 CONCURRENTLY.

If a REINDEX TABLE CONCURRENTLY fails on a table having a TOAST table, an
invalid index will be present for the TOAST table.  As we only allow to drop
invalid indexes on TOAST tables, reindexing those would lead to useless
duplicated indexes that can't be dropped anymore.

Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
---
 .../expected/reindex-concurrently.out         | 54 ++++++++++++++++++-
 .../isolation/specs/reindex-concurrently.spec | 19 +++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..69a55d3788 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
 
 starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,55 @@ step end1: COMMIT;
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
 step end2: COMMIT;
 step reindex: <... completed>
+
+starting permutation: check_invalid lock reindex_timeout unlock check_invalid nowarn normal_reindex check_invalid reindex check_invalid
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+t              
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data           
+
+aa             
+isolationtester: canceling step reindex_timeout after 1 seconds
+step reindex_timeout: REINDEX TABLE CONCURRENTLY reind_con_tab;
+ERROR:  canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step nowarn: SET client_min_messages = 'ERROR';
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..dc67e2afd2 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
 
 session "s3"
 step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "reindex_timeout" { REINDEX TABLE CONCURRENTLY reind_con_tab; } timeout 1
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C"; }
 
 permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
 permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,6 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+permutation "check_invalid" "lock" "reindex_timeout" "unlock" "check_invalid"
+            "nowarn" "normal_reindex" "check_invalid"
+            "reindex" "check_invalid"
-- 
2.25.1

From 2e808a2971fcaf160f6a1e6c9c80366ff053bccf Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 9 Mar 2020 14:43:33 +0900
Subject: [PATCH v2] Forbid reindex of invalid indexes on TOAST tables

Such indexes can only be duplicated leftovers of a previously failed
REINDEX CONCURRENTLY command, and a valid equivalent is guaranteed to
exist already.  As we only allow the drop of invalid indexes on TOAST
tables, reindexing these would lead to useless duplicated indexes that
can't be dropped anymore.

Thanks to Justin Pryzby for reminding that this problem was reported
long ago, but it has never been addressed.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
 src/include/utils/lsyscache.h       |  1 +
 src/backend/catalog/index.c         | 28 ++++++++++++++++++++++++++++
 src/backend/commands/indexcmds.c    | 12 ++++++++++++
 src/backend/utils/cache/lsyscache.c | 23 +++++++++++++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index f132d39458..131d10eab0 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -181,6 +181,7 @@ extern char *get_namespace_name_or_temp(Oid nspid);
 extern Oid	get_range_subtype(Oid rangeOid);
 extern Oid	get_range_collation(Oid rangeOid);
 extern Oid	get_index_column_opclass(Oid index_oid, int attno);
+extern bool get_index_isvalid(Oid index_oid);
 
 #define type_is_array(typid)  (get_element_type(typid) != InvalidOid)
 /* type_is_array_domain accepts both plain arrays and domains over arrays */
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..ec2d7dc9cb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
 #include "catalog/pg_depend.h"
 #include "catalog/pg_description.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_tablespace.h"
@@ -3474,6 +3475,17 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot reindex temporary tables of other sessions")));
 
+	/*
+	 * Don't allow reindex on an invalid toast index.  This is a leftover from
+	 * a failed REINDEX CONCURRENTLY, and if rebuilt it would not be possible
+	 * to drop it anymore.
+	 */
+	if (RelationGetNamespace(iRel) == PG_TOAST_NAMESPACE &&
+		!get_index_isvalid(indexId))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("cannot reindex invalid toast index")));
+
 	/*
 	 * Also check for active uses of the index in the current transaction; we
 	 * don't want to reindex underneath an open indexscan.
@@ -3724,6 +3736,22 @@ reindex_relation(Oid relid, int flags, int options)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
 
+			/*
+			 * Skip any invalid indexes on a TOAST table.  These can only be
+			 * duplicate leftovers from a failed REINDEX CONCURRENTLY, and if
+			 * rebuilt it it won't be possible to drop them anymore.
+			 */
+			if (get_rel_namespace(indexOid) == PG_TOAST_NAMESPACE &&
+				!get_index_isvalid(indexOid))
+			{
+				ereport(WARNING,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("cannot reindex invalid toast index \"%s.%s\", skipping",
+								get_namespace_name(get_rel_namespace(indexOid)),
+								get_rel_name(indexOid))));
+				continue;
+			}
+
 			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
 						  persistence, options);
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3f3a89fe92..6f1dee8643 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
 #include "catalog/pg_am.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_opfamily.h"
 #include "catalog/pg_tablespace.h"
@@ -2868,6 +2869,17 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("cannot reindex system catalogs concurrently")));
 
+				/*
+				 * Don't allow reindex for an invalid toast index, as if rebuild
+				 * it would not be possible to drop it.  Its valid equivalent also
+				 * already exists.
+				 */
+				if (get_rel_namespace(relationOid) ==  PG_TOAST_NAMESPACE &&
+					!get_index_isvalid(relationOid))
+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("cannot reindex invalid toast index concurrently")));
+
 				/* Save the list of relation OIDs in private context */
 				oldcontext = MemoryContextSwitchTo(private_context);
 
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 3da90cb72a..400e7689fe 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3227,3 +3227,26 @@ get_index_column_opclass(Oid index_oid, int attno)
 
 	return opclass;
 }
+
+/*
+ * get_index_isvalid
+ *
+ *		Given the index OID, return pg_index.indisvalid.
+ */
+bool
+get_index_isvalid(Oid index_oid)
+{
+	bool		isvalid;
+	HeapTuple	tuple;
+	Form_pg_index rd_index;
+
+	tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+	rd_index = (Form_pg_index) GETSTRUCT(tuple);
+	isvalid = rd_index->indisvalid;
+	ReleaseSysCache(tuple);
+
+	return isvalid;
+}
-- 
2.25.1

From fd988b4141725082b30148da31d9c2a6a88c7319 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 10 Mar 2020 12:01:23 +0900
Subject: [PATCH] Add isolation test to check dependency handling of invalid
 indexes

This uses a statement_timeout of 1s, which is not really a good idea :D
---
 .../isolation/expected/reindex-invalid.out    | 34 +++++++++++++++++
 src/test/isolation/isolation_schedule         |  1 +
 src/test/isolation/specs/reindex-invalid.spec | 38 +++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 src/test/isolation/expected/reindex-invalid.out
 create mode 100644 src/test/isolation/specs/reindex-invalid.spec

diff --git a/src/test/isolation/expected/reindex-invalid.out b/src/test/isolation/expected/reindex-invalid.out
new file mode 100644
index 0000000000..ef1eff9ce7
--- /dev/null
+++ b/src/test/isolation/expected/reindex-invalid.out
@@ -0,0 +1,34 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_select s2_timeout s2_reindex s2_check s1_commit s1_drop s2_check
+step s1_select: SELECT data FROM reind_con_invalid FOR UPDATE;
+data           
+
+foo            
+bar            
+step s2_timeout: SET statement_timeout = 1000;
+step s2_reindex: REINDEX TABLE CONCURRENTLY reind_con_invalid; <waiting ...>
+step s2_reindex: <... completed>
+ERROR:  canceling statement due to statement timeout
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid = c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid = t.oid
+					    WHERE c.relname = 'reind_con_invalid'
+					      ORDER BY c.relname;
+regexp_replace indisvalid     
+
+pg_toast.pg_toast_<oid>_index_ccoldf              
+pg_toast.pg_toast_<oid>_indext              
+step s1_commit: COMMIT;
+step s1_drop: DROP TABLE reind_con_invalid;
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid = c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid = t.oid
+					    WHERE c.relname = 'reind_con_invalid'
+					      ORDER BY c.relname;
+regexp_replace indisvalid     
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 2873cd7c21..28e5b8b16a 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -48,6 +48,7 @@ test: lock-committed-update
 test: lock-committed-keyupdate
 test: update-locked-tuple
 test: reindex-concurrently
+test: reindex-invalid
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-invalid.spec b/src/test/isolation/specs/reindex-invalid.spec
new file mode 100644
index 0000000000..bf866d2279
--- /dev/null
+++ b/src/test/isolation/specs/reindex-invalid.spec
@@ -0,0 +1,38 @@
+# REINDEX CONCURRENTLY with invalid indexes
+#
+# Check that handling of dependencies with invalid indexes is correct.
+# When the parent table is dropped, all the indexes are dropped.
+
+setup
+{
+	CREATE TABLE reind_con_invalid (id serial primary key, data text);
+	INSERT INTO reind_con_invalid (data) VALUES ('foo');
+	INSERT INTO reind_con_invalid (data) VALUES ('bar');
+}
+
+# No need to drop the table at teardown phase here as each permutation
+# takes care of it internally.
+
+session "s1"
+setup { BEGIN; }
+step "s1_select" { SELECT data FROM reind_con_invalid FOR UPDATE; }
+step "s1_commit" { COMMIT; }
+step "s1_drop"   { DROP TABLE reind_con_invalid; }
+
+session "s2"
+step "s2_timeout" { SET statement_timeout = 1000; }
+step "s2_reindex" { REINDEX TABLE CONCURRENTLY reind_con_invalid; }
+step "s2_check" { SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid = c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid = t.oid
+					    WHERE c.relname = 'reind_con_invalid'
+					      ORDER BY c.relname; }
+
+# A failed REINDEX CONCURRENTLY will leave an invalid index on the toast
+# table of TOAST table.  Any following successful REINDEX should leave
+# this index as invalid, otherwise we would end up with a useless and
+# duplicated index that can't be dropped.  Any invalid index should be
+# dropped once the parent table is dropped.
+permutation "s1_select" "s2_timeout" "s2_reindex" "s2_check" "s1_commit" "s1_drop" "s2_check"
-- 
2.25.1

From 77c214c9fb2fa7f9a003b96db0e5ec6506217e38 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:40:56 +0100
Subject: [PATCH v2 1/2] Add an optional cancel on clause to isolationtester
 steps.

Some sanity checks can require a command to wait on a lock and eventually be
cancelled.  The only way to do that was to rely on calls to pg_cancel_backend()
filtering pg_stat_activity view, but this isn't a satisfactory solution as
there's no way to guarantee that only the wanted backend will be canceled.

Instead, add a new optional "cancel on <wait_event_type> <wait_event>" clause
to the step definition.  When this clause is specified, isolationtester will
actively wait on that command, and issue a cancel when the query is waiting on
the given wait event.

Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200305035354.GQ2593%40paquier.xyz
---
 src/backend/utils/adt/lockfuncs.c    | 54 ++++++++++++++++++++++++++--
 src/include/catalog/pg_proc.dat      |  5 ++-
 src/test/isolation/README            | 12 +++++--
 src/test/isolation/isolationtester.c | 43 +++++++++++++++++-----
 src/test/isolation/isolationtester.h |  8 +++++
 src/test/isolation/specparse.y       | 20 +++++++++--
 src/test/isolation/specscanner.l     |  2 ++
 7 files changed, 127 insertions(+), 17 deletions(-)

diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index ecb1bf92ff..cc2bc94d0d 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -17,7 +17,9 @@
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "pgstat.h"
 #include "storage/predicate_internals.h"
+#include "storage/procarray.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 
@@ -578,17 +580,28 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
 Datum
 pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 {
+	TupleDesc	tupdesc;
 	int			blocked_pid = PG_GETARG_INT32(0);
 	ArrayType  *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1);
 	ArrayType  *blocking_pids_a;
 	int32	   *interesting_pids;
 	int32	   *blocking_pids;
+	Datum		values[3];
+	bool		nulls[3];
+	PGPROC	   *proc;
+	uint32		raw_wait_event = 0;
+	const char *wait_event_type = NULL;
+	const char *wait_event = NULL;
 	int			num_interesting_pids;
 	int			num_blocking_pids;
 	int			dummy;
 	int			i,
 				j;
 
+	/* Initialise values and NULL flags arrays */
+	MemSet(values, 0, sizeof(values));
+	MemSet(nulls, 0, sizeof(nulls));
+
 	/* Validate the passed-in array */
 	Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID);
 	if (array_contains_nulls(interesting_pids_a))
@@ -597,6 +610,34 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 	num_interesting_pids = ArrayGetNItems(ARR_NDIM(interesting_pids_a),
 										  ARR_DIMS(interesting_pids_a));
 
+	/* Initialise attributes information in the tuple descriptor */
+	tupdesc = CreateTemplateTupleDesc(3);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "blocked",
+					   BOOLOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "wait_event_type",
+					   TEXTOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 3, "wait_even",
+					   TEXTOID, -1, 0);
+
+	BlessTupleDesc(tupdesc);
+
+	proc = BackendPidGetProc(blocked_pid);
+	if (proc)
+	{
+#define UINT32_ACCESS_ONCE(var)		 ((uint32)(*((volatile uint32 *)&(var))))
+		raw_wait_event = UINT32_ACCESS_ONCE(proc->wait_event_info);
+		wait_event_type = pgstat_get_wait_event_type(raw_wait_event);
+		wait_event = pgstat_get_wait_event(raw_wait_event);
+
+		if (wait_event_type != NULL)
+			values[1] = CStringGetTextDatum(wait_event_type);
+		if (wait_event != NULL)
+			values[2] = CStringGetTextDatum(wait_event);
+	}
+
+	nulls[1] = (wait_event_type == NULL);
+	nulls[2] = (wait_event == NULL);
+
 	/*
 	 * Get the PIDs of all sessions blocking the given session's attempt to
 	 * acquire heavyweight locks.
@@ -623,7 +664,11 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 		for (j = 0; j < num_interesting_pids; j++)
 		{
 			if (blocking_pids[i] == interesting_pids[j])
-				PG_RETURN_BOOL(true);
+			{
+				values[0] = BoolGetDatum(true);
+				PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc,
+								values, nulls)));
+			}
 		}
 
 	/*
@@ -636,9 +681,12 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 	 * buffer and check if the number of safe snapshot blockers is non-zero.
 	 */
 	if (GetSafeSnapshotBlockingPids(blocked_pid, &dummy, 1) > 0)
-		PG_RETURN_BOOL(true);
+		values[0] = BoolGetDatum(true);
+
+	values[0] = BoolGetDatum(false);
 
-	PG_RETURN_BOOL(false);
+	PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc,
+					values, nulls)));
 }
 
 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7fb574f9dc..77529e181d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5869,7 +5869,10 @@
   prosrc => 'pg_safe_snapshot_blocking_pids' },
 { oid => '3378', descr => 'isolationtester support function',
   proname => 'pg_isolation_test_session_is_blocked', provolatile => 'v',
-  prorettype => 'bool', proargtypes => 'int4 _int4',
+  prorettype => 'record', proargtypes => 'int4 _int4',
+  proallargtypes => '{int4,_int4,bool,text,text}',
+  proargmodes => '{i,i,o,o,o}',
+  proargnames => '{blocking_pid,interesting_pids,blocked,wait_event_type,wait_event}',
   prosrc => 'pg_isolation_test_session_is_blocked' },
 { oid => '1065', descr => 'view two-phase transactions',
   proname => 'pg_prepared_xact', prorows => '1000', proretset => 't',
diff --git a/src/test/isolation/README b/src/test/isolation/README
index 217953d183..04aed5cd17 100644
--- a/src/test/isolation/README
+++ b/src/test/isolation/README
@@ -86,10 +86,12 @@ session "<name>"
 
   Each step has the syntax
 
-  step "<name>" { <SQL> }
+  step "<name>" { <SQL> } [ cancel on "<wait_event_type>" "<wait_event>" ]
 
-  where <name> is a name identifying this step, and SQL is a SQL statement
-  (or statements, separated by semicolons) that is executed in the step.
+  where <name> is a name identifying this step, SQL is a SQL statement
+  (or statements, separated by semicolons) that is executed in the step and
+  <wait_event_type> and <wait_event> a wait event specification for which
+  isolationtester will cancel the query if it's blocked on it.
   Step names must be unique across the whole spec file.
 
 permutation "<step name>" ...
@@ -125,6 +127,10 @@ after PGISOLATIONTIMEOUT seconds.  If the cancel doesn't work, isolationtester
 will exit uncleanly after a total of twice PGISOLATIONTIMEOUT.  Testing
 invalid permutations should be avoided because they can make the isolation
 tests take a very long time to run, and they serve no useful testing purpose.
+If a test specified the optionnal cancel on specification, then isolationtester
+will actively wait for the step commands completion rather than continuing with
+the permutation next step, and send a cancel once the given wait event is
+blocking the query.
 
 Note that isolationtester recognizes that a command has blocked by looking
 to see if it is shown as waiting in the pg_locks view; therefore, only
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index f80261c022..f530d9923d 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -43,6 +43,7 @@ static void run_permutation(TestSpec *testspec, int nsteps, Step **steps);
 
 #define STEP_NONBLOCK	0x1		/* return 0 as soon as cmd waits for a lock */
 #define STEP_RETRY		0x2		/* this is a retry of a previously-waiting cmd */
+#define STEP_WAIT		0x4		/* active wait for a given wait event */
 static bool try_complete_step(TestSpec *testspec, Step *step, int flags);
 
 static int	step_qsort_cmp(const void *a, const void *b);
@@ -212,7 +213,7 @@ main(int argc, char **argv)
 	 */
 	initPQExpBuffer(&wait_query);
 	appendPQExpBufferStr(&wait_query,
-						 "SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
+						 "SELECT * FROM pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
 	/* The spec syntax requires at least one session; assume that here. */
 	appendPQExpBufferStr(&wait_query, backend_pid_strs[1]);
 	for (i = 2; i < nconns; i++)
@@ -587,8 +588,14 @@ run_permutation(TestSpec *testspec, int nsteps, Step **steps)
 			exit(1);
 		}
 
-		/* Try to complete this step without blocking.  */
-		mustwait = try_complete_step(testspec, step, STEP_NONBLOCK);
+		/*
+		 * Try to complete this step without blocking, unless the step has a
+		 * cancel on clause.
+		 */
+		mustwait = try_complete_step(testspec, step,
+				(step->waitinfo ? STEP_WAIT : STEP_NONBLOCK));
+		if (step->waitinfo)
+			report_error_message(step);
 
 		/* Check for completion of any steps that were previously waiting. */
 		w = 0;
@@ -720,10 +727,12 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 		else if (ret == 0)		/* select() timeout: check for lock wait */
 		{
 			struct timeval current_time;
+			char  *wait_event_type = "";
+			char  *wait_event = "";
 			int64		td;
 
 			/* If it's OK for the step to block, check whether it has. */
-			if (flags & STEP_NONBLOCK)
+			if (flags & (STEP_NONBLOCK | STEP_WAIT))
 			{
 				bool		waiting;
 
@@ -738,9 +747,17 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 					exit(1);
 				}
 				waiting = ((PQgetvalue(res, 0, 0))[0] == 't');
+				if (waiting && (flags & STEP_WAIT))
+				{
+					wait_event_type = pg_strdup(PQgetvalue(res, 0, 1));
+					Assert(wait_event_type != NULL);
+
+					wait_event = pg_strdup(PQgetvalue(res, 0, 2));
+					Assert(wait_event != NULL);
+				}
 				PQclear(res);
 
-				if (waiting)	/* waiting to acquire a lock */
+				if (waiting && (flags & STEP_NONBLOCK))	/* waiting to acquire a lock */
 				{
 					/*
 					 * Since it takes time to perform the lock-check query,
@@ -787,7 +804,11 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 			 * failing, but remaining permutations and tests should still be
 			 * OK.
 			 */
-			if (td > max_step_wait && !canceled)
+			if ((td > max_step_wait ||
+				(step->waitinfo
+				 && strcmp(step->waitinfo->wait_event_type, wait_event_type) == 0
+				 && strcmp(step->waitinfo->wait_event, wait_event) == 0))
+				&& !canceled)
 			{
 				PGcancel   *cancel = PQgetCancel(conn);
 
@@ -801,8 +822,14 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
 						 * print to stdout not stderr, as this should appear
 						 * in the test case's results
 						 */
-						printf("isolationtester: canceling step %s after %d seconds\n",
-							   step->name, (int) (td / USECS_PER_SEC));
+						if (!step->waitinfo)
+							printf("isolationtester: canceling step %s after %d seconds\n",
+								   step->name, (int) (td / USECS_PER_SEC));
+						else
+							printf("isolationtester: canceling step %s on wait event %s/%s\n",
+									step->name,
+									step->waitinfo->wait_event_type,
+									step->waitinfo->wait_event);
 						canceled = true;
 					}
 					else
diff --git a/src/test/isolation/isolationtester.h b/src/test/isolation/isolationtester.h
index 9cf5012416..5df540485c 100644
--- a/src/test/isolation/isolationtester.h
+++ b/src/test/isolation/isolationtester.h
@@ -26,6 +26,12 @@ struct Session
 	int			nsteps;
 };
 
+typedef struct WaitInfo
+{
+	char	   *wait_event_type;
+	char	   *wait_event;
+} WaitInfo;
+
 struct Step
 {
 	int			session;
@@ -33,6 +39,8 @@ struct Step
 	char	   *name;
 	char	   *sql;
 	char	   *errormsg;
+	WaitInfo   *waitinfo;
+	struct timeval start_time;
 };
 
 typedef struct
diff --git a/src/test/isolation/specparse.y b/src/test/isolation/specparse.y
index 5e007e1bf0..c8e72f4316 100644
--- a/src/test/isolation/specparse.y
+++ b/src/test/isolation/specparse.y
@@ -27,6 +27,7 @@ TestSpec		parseresult;			/* result of parsing is left here */
 	char	   *str;
 	Session	   *session;
 	Step	   *step;
+	WaitInfo   *waitinfo;
 	Permutation *permutation;
 	struct
 	{
@@ -43,9 +44,11 @@ TestSpec		parseresult;			/* result of parsing is left here */
 %type <session> session
 %type <step> step
 %type <permutation> permutation
+%type <waitinfo> opt_cancel
 
 %token <str> sqlblock string_literal
-%token PERMUTATION SESSION SETUP STEP TEARDOWN TEST
+%token <str> LITERAL
+%token CANCEL ON PERMUTATION SESSION SETUP STEP TEARDOWN TEST
 
 %%
 
@@ -140,16 +143,29 @@ step_list:
 
 
 step:
-			STEP string_literal sqlblock
+			STEP string_literal sqlblock opt_cancel
 			{
 				$$ = pg_malloc(sizeof(Step));
 				$$->name = $2;
 				$$->sql = $3;
 				$$->used = false;
 				$$->errormsg = NULL;
+				$$->waitinfo = $4;
 			}
 		;
 
+opt_cancel:
+			CANCEL ON string_literal string_literal
+			{
+				$$ = pg_malloc(sizeof(WaitInfo));
+				$$->wait_event_type = $3;
+				$$->wait_event = $4;
+			}
+			| /* EMPTY */
+			{
+				$$ = NULL;
+			}
+		;
 
 opt_permutation_list:
 			permutation_list
diff --git a/src/test/isolation/specscanner.l b/src/test/isolation/specscanner.l
index 410f17727e..672e5fa2fc 100644
--- a/src/test/isolation/specscanner.l
+++ b/src/test/isolation/specscanner.l
@@ -48,6 +48,8 @@ comment			("#"{non_newline}*)
 	litbufsize = LITBUF_INIT;
 %}
 
+cancel			{ return CANCEL; }
+on				{ return ON; }
 permutation		{ return PERMUTATION; }
 session			{ return SESSION; }
 setup			{ return SETUP; }
-- 
2.25.1

From 60267b2fb3ff8e3b632ee07d12ee4031543601d3 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 6 Mar 2020 13:51:35 +0100
Subject: [PATCH v2 2/2] Add regression tests for failed REINDEX TABLE
 CONCURRENTLY.

If a REINDEX TABLE CONCURRENTLY fails on a table having a TOAST table, an
invalid index will be present for the TOAST table.  As we only allow to drop
invalid indexes on TOAST tables, reindexing those would lead to useless
duplicated indexes that can't be dropped anymore.

Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
---
 .../expected/reindex-concurrently.out         | 54 ++++++++++++++++++-
 .../isolation/specs/reindex-concurrently.spec | 19 +++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..674a8890bd 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
 
 starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,55 @@ step end1: COMMIT;
 step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
 step end2: COMMIT;
 step reindex: <... completed>
+
+starting permutation: check_invalid lock reindex_fail unlock check_invalid nowarn normal_reindex check_invalid reindex check_invalid
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+t              
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data           
+
+aa             
+isolationtester: canceling step reindex_fail on wait event Lock/virtualxid
+step reindex_fail: REINDEX TABLE CONCURRENTLY reind_con_tab;
+ERROR:  canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step nowarn: SET client_min_messages = 'ERROR';
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid     
+
+f              
+t              
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..a3cd8b72a5 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
 
 session "s3"
 step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "reindex_fail" { REINDEX TABLE CONCURRENTLY reind_con_tab; } cancel on "Lock" "virtualxid"
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'ERROR'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT i.indisvalid
+    FROM pg_class c
+    JOIN pg_class t ON t.oid = c.reltoastrelid
+    JOIN pg_index i ON i.indrelid = t.oid
+    WHERE c.relname = 'reind_con_tab'
+    ORDER BY i.indisvalid::text COLLATE "C"; }
 
 permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
 permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,6 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
 permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+permutation "check_invalid" "lock" "reindex_fail" "unlock" "check_invalid"
+            "nowarn" "normal_reindex" "check_invalid"
+            "reindex" "check_invalid"
-- 
2.25.1