Date: Fri, 6 Mar 2020 13:36:48 +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: <20200306123648.GA49431@nol>
References: <20200222150657.GA54846@nol>
 <20200227073211.GA403330@paquier.xyz>
 <20200227080735.l32fqcauy73lon7o@nol>
 <20200303080642.GA1890@paquier.xyz>
 <20200303092551.GB1890@paquier.xyz>
 <20200304051510.GE2593@paquier.xyz>
 <CAOBaU_Z=qkE42VQ25mps+zyAz+WUn3wZoev4uhGW+gAXBBS2YA@mail.gmail.com>
 <20200305035354.GQ2593@paquier.xyz>
 <20200305165707.GA35281@nol>
 <20200306013844.GE52814@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="M9NhX3UHpAaciwkO"
Content-Disposition: inline
In-Reply-To: <20200306013844.GE52814@paquier.xyz>
Precedence: bulk


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

On Fri, Mar 06, 2020 at 10:38:44AM +0900, Michael Paquier wrote:
> On Thu, Mar 05, 2020 at 05:57:07PM +0100, Julien Rouhaud wrote:
> > I agree that the approach wasn't quite robust.  I'll try to look at adding a
> > new command for isolationtester, but that's probably not something we want to
> > put in pg13?
>
> Yes, that's too late.
>
> > Note that while looking at it, I noticed another bug in RIC:
> >
> > [...]
> >
> > # reindex table concurrently t1;
> > WARNING:  0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
> > LOCATION:  ReindexRelationConcurrently, indexcmds.c:2821
> > WARNING:  XX002: cannot reindex invalid index "pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
> > LOCATION:  ReindexRelationConcurrently, indexcmds.c:2867
> > REINDEX
> > # reindex index concurrently t1_val_idx_ccold;
> > REINDEX
> >
> > That case is also fixed in this patch.
>
> This choice is intentional.  The idea about bypassing invalid indexes
> for table-level REINDEX is that this would lead to a bloat in the
> number of relations to handling if multiple runs are failing, leading
> to more and more invalid indexes to handle each time.  Allowing a
> single invalid non-toast index to be reindexed with CONCURRENTLY can
> be helpful in some cases, like for example a CIC for a unique index
> that failed and was invalid, where the relation already defined can be
> reused.

Ah I see, thanks for the clarification.  I guess there's room for improvement
in the comments about that, since the ERRCODE_FEATURE_NOT_SUPPORTED usage is
quite misleading there.

v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
non-TOAST index anymore.

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

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


--M9NhX3UHpAaciwkO--