On Fri, Mar 06, 2020 at 01:36:48PM +0100, Julien Rouhaud wrote: > 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.
Thanks. The position of the error check in reindex_relation() is correct, but as it opens a relation for the cache lookup let's invent a new routine in lsyscache.c to grab pg_index.indisvalid. It is possible to make use of this routine with all the other checks: - WARNING for REINDEX TABLE (non-conurrent) - ERROR for REINDEX INDEX (non-conurrent) - ERROR for REINDEX INDEX CONCURRENTLY (There is already a WARNING for REINDEX TABLE CONCURRENTLY.) I did not find the addition of an error check in ReindexIndex() consistent with the existing practice to check the state of the relation reindexed in reindex_index() (for the non-concurrent case) and ReindexRelationConcurrently() (for the concurrent case). Okay, this leads to the introduction of two new ERROR messages related to invalid toast indexes for the concurrent and the non-concurrent cases when using REINDEX INDEX instead of one, but having two messages leads to something much more consistent with the rest, and all checks remain centralized in the same routines. For the index-level operation, issuing a WARNING is not consistent with the existing practice to use an ERROR, which is more adapted as the operation is done on a single index at a time. For the check in reindex_relation, it is more consistent to check the namespace of the index instead of the parent relation IMO (the previous patch used "rel", which refers to the parent table). This has in practice no consequence though. It would have been nice to test this stuff. However, this requires an invalid toast index and we cannot create that except by canceling a concurrent reindex, leading us back to the upthread discussion about isolation tests, timeouts and fault injection :/ Any opinions? -- Michael
From 2e808a2971fcaf160f6a1e6c9c80366ff053bccf Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@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.ga21...@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
signature.asc
Description: PGP signature
