Date: Sat, 22 Feb 2020 16:06:57 +0100
From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Sergei Kornilov <sk@zsrv.org>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	Michael Paquier <michael.paquier@gmail.com>,
	Andreas Karlsson <andreas@proxel.se>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: reindex concurrently and two toast indexes
Message-ID: <20200222150657.GA54846@nol>
References: <20200216190835.GA21832@telsasoft.com>
 <20200218052933.GH4176@paquier.xyz>
 <CAOBaU_Z1-uD1nbnQwJXgSc2xYvPWe=Cn5vJwyAjOCd+tWfxt5w@mail.gmail.com>
 <20200218061913.GJ4176@paquier.xyz>
 <CAOBaU_bieK_PPD-NzBF1jR+QCNeJ75vZu_6hqCq_zbZHsDijgQ@mail.gmail.com>
 <20200222070924.GA57285@nol>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="YiEDa0DAkWCtVeE4"
Content-Disposition: inline
In-Reply-To: <20200222070924.GA57285@nol>
Precedence: bulk


--YiEDa0DAkWCtVeE4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Feb 22, 2020 at 08:09:24AM +0100, Julien Rouhaud wrote:
> On Tue, Feb 18, 2020 at 07:39:49AM +0100, Julien Rouhaud wrote:
> > On Tue, Feb 18, 2020 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> > > > On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > >> Hmm.  There could be an argument here for skipping invalid toast
> > > >> indexes within reindex_index(), because we are sure about having at
> > > >> least one valid toast index at anytime, and these are not concerned
> > > >> with CIC.
>
> PFA a patch to fix the problem using this approach.
>
> I also added isolation tester regression tests.  The failure is simulated using
> a pg_cancel_backend() on top of pg_stat_activity, using filters on a
> specifically set application name and the query text to avoid any unwanted
> interaction.  I also added a 1s locking delay, to ensure that even slow/CCA
> machines can consistently reproduce the failure.  Maybe that's not enough, or
> maybe testing this scenario is not worth the extra time.

Sorry, I just realized that I forgot to commit the last changes before sending
the patch, so here's the correct v2.

--YiEDa0DAkWCtVeE4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v2.patch"

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


--YiEDa0DAkWCtVeE4--