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

