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.
>From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001 From: Julien Rouhaud <julien.rouh...@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.ga21...@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
