From 28847ae77d8947f32fa96cb68f7f3d1b5ca2ae30 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 19 Sep 2003 19:57:42 +0000 Subject: [PATCH] Seems like a bad idea that REINDEX TABLE supports (or thinks it does) reindexing system tables without ignoring system indexes, when the other two varieties of REINDEX disallow it. Make all three act the same, and simplify downstream code accordingly. --- src/backend/catalog/index.c | 122 +++++++++---------------------- src/backend/commands/indexcmds.c | 32 +++++++- 2 files changed, 63 insertions(+), 91 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 9c16e3e023..c3ff6ded38 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.214 2003/08/04 02:39:58 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/catalog/index.c,v 1.215 2003/09/19 19:57:42 tgl Exp $ * * * INTERFACE ROUTINES @@ -76,7 +76,6 @@ static void UpdateIndexRelation(Oid indexoid, Oid heapoid, Oid *classOids, bool primary); static Oid IndexGetRelation(Oid indexId); -static bool activate_index(Oid indexId, bool activate, bool inplace); static bool reindexing = false; @@ -1690,23 +1689,8 @@ IndexGetRelation(Oid indexId) return result; } -/* --------------------------------- - * activate_index -- activate/deactivate the specified index. - * Note that currently PostgreSQL doesn't hold the - * status per index - * --------------------------------- - */ -static bool -activate_index(Oid indexId, bool activate, bool inplace) -{ - if (!activate) /* Currently does nothing */ - return true; - return reindex_index(indexId, false, inplace); -} - -/* -------------------------------- +/* * reindex_index - This routine is used to recreate an index - * -------------------------------- */ bool reindex_index(Oid indexId, bool force, bool inplace) @@ -1745,22 +1729,24 @@ reindex_index(Oid indexId, bool force, bool inplace) * the relcache can't cope with changing its relfilenode. * * In either of these cases, we are definitely processing a system index, - * so we'd better be ignoring system indexes. + * so we'd better be ignoring system indexes. (These checks are just + * for paranoia's sake --- upstream code should have disallowed reindex + * in such cases already.) */ if (iRel->rd_rel->relisshared) { if (!IsIgnoringSystemIndexes()) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("the target relation %u is shared", indexId))); + elog(ERROR, + "must be ignoring system indexes to reindex shared index %u", + indexId); inplace = true; } if (iRel->rd_isnailed) { if (!IsIgnoringSystemIndexes()) - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("the target relation %u is nailed", indexId))); + elog(ERROR, + "must be ignoring system indexes to reindex nailed index %u", + indexId); inplace = true; } @@ -1801,13 +1787,12 @@ reindex_index(Oid indexId, bool force, bool inplace) return true; } +#ifdef NOT_USED /* - * ---------------------------- * activate_indexes_of_a_table * activate/deactivate indexes of the specified table. * * Caller must already hold exclusive lock on the table. - * ---------------------------- */ bool activate_indexes_of_a_table(Relation heaprel, bool activate) @@ -1829,11 +1814,11 @@ activate_indexes_of_a_table(Relation heaprel, bool activate) } return true; } +#endif /* NOT_USED */ -/* -------------------------------- - * reindex_relation - This routine is used to recreate indexes +/* + * reindex_relation - This routine is used to recreate all indexes * of a relation. - * -------------------------------- */ bool reindex_relation(Oid relid, bool force) @@ -1844,11 +1829,10 @@ reindex_relation(Oid relid, bool force) HeapTuple indexTuple; bool old, reindexed; - bool deactivate_needed, - overwrite; + bool overwrite; Relation rel; - overwrite = deactivate_needed = false; + overwrite = false; /* * Ensure to hold an exclusive lock throughout the transaction. The @@ -1858,12 +1842,14 @@ reindex_relation(Oid relid, bool force) rel = heap_open(relid, AccessExclusiveLock); /* - * ignore the indexes of the target system relation while processing - * reindex. + * Should be ignoring system indexes if we are reindexing a system table. + * (This is elog not ereport because caller should have caught it.) */ if (!IsIgnoringSystemIndexes() && IsSystemRelation(rel) && !IsToastRelation(rel)) - deactivate_needed = true; + elog(ERROR, + "must be ignoring system indexes to reindex system table %u", + relid); /* * Shared system indexes must be overwritten because it's impossible @@ -1871,49 +1857,35 @@ reindex_relation(Oid relid, bool force) */ if (rel->rd_rel->relisshared) { - if (IsIgnoringSystemIndexes()) - { - overwrite = true; - deactivate_needed = true; - } - else - ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("the target relation %u is shared", relid))); + if (!IsIgnoringSystemIndexes()) /* shouldn't happen */ + elog(ERROR, + "must be ignoring system indexes to reindex shared table %u", + relid); + overwrite = true; } old = SetReindexProcessing(true); - if (deactivate_needed) - { - if (IndexesAreActive(rel)) - { - if (!force) - { - SetReindexProcessing(old); - heap_close(rel, NoLock); - return false; - } - activate_indexes_of_a_table(rel, false); - CommandCounterIncrement(); - } - } - /* * Continue to hold the lock. */ heap_close(rel, NoLock); + /* + * Find table's indexes by looking in pg_index (not trusting indexes...) + */ indexRelation = heap_openr(IndexRelationName, AccessShareLock); - ScanKeyEntryInitialize(&entry, 0, Anum_pg_index_indrelid, - F_OIDEQ, ObjectIdGetDatum(relid)); + ScanKeyEntryInitialize(&entry, 0, + Anum_pg_index_indrelid, + F_OIDEQ, + ObjectIdGetDatum(relid)); scan = heap_beginscan(indexRelation, SnapshotNow, 1, &entry); reindexed = false; while ((indexTuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_index index = (Form_pg_index) GETSTRUCT(indexTuple); - if (activate_index(index->indexrelid, true, overwrite)) + if (reindex_index(index->indexrelid, false, overwrite)) reindexed = true; else { @@ -1923,31 +1895,7 @@ reindex_relation(Oid relid, bool force) } heap_endscan(scan); heap_close(indexRelation, AccessShareLock); - if (reindexed) - { - /* - * Ok,we could use the reindexed indexes of the target system - * relation now. - */ - if (deactivate_needed) - { - if (!overwrite && relid == RelOid_pg_class) - { - /* - * For pg_class, relhasindex should be set to true here in - * place. - */ - setRelhasindex(relid, true, false, InvalidOid); - CommandCounterIncrement(); - /* - * However the following setRelhasindex() is needed to - * keep consistency with WAL. - */ - } - setRelhasindex(relid, true, false, InvalidOid); - } - } SetReindexProcessing(old); return reindexed; diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 45f4c3b4f7..3021eeaf4c 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.106 2003/08/17 19:58:04 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.107 2003/09/19 19:57:42 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -658,17 +658,41 @@ void ReindexTable(RangeVar *relation, bool force) { Oid heapOid; - char relkind; + HeapTuple tuple; heapOid = RangeVarGetRelid(relation, false); - relkind = get_rel_relkind(heapOid); + tuple = SearchSysCache(RELOID, + ObjectIdGetDatum(heapOid), + 0, 0, 0); + if (!HeapTupleIsValid(tuple)) /* shouldn't happen */ + elog(ERROR, "cache lookup failed for relation %u", heapOid); - if (relkind != RELKIND_RELATION && relkind != RELKIND_TOASTVALUE) + if (((Form_pg_class) GETSTRUCT(tuple))->relkind != RELKIND_RELATION && + ((Form_pg_class) GETSTRUCT(tuple))->relkind != RELKIND_TOASTVALUE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("relation \"%s\" is not a table", relation->relname))); + if (IsSystemClass((Form_pg_class) GETSTRUCT(tuple)) && + !IsToastClass((Form_pg_class) GETSTRUCT(tuple))) + { + if (!allowSystemTableMods) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a system table", + relation->relname), + errhint("Do REINDEX in standalone postgres with -O -P options."))); + if (!IsIgnoringSystemIndexes()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a system table", + relation->relname), + errhint("Do REINDEX in standalone postgres with -P -O options."))); + } + + ReleaseSysCache(tuple); + /* * In-place REINDEX within a transaction block is dangerous, because * if the transaction is later rolled back we have no way to undo