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

@@ -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,
+			elog(ERROR,
-					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 "must be ignoring system indexes to reindex shared index %u",
-				   errmsg("the target relation %u is shared", indexId)));
+				 indexId);
 		inplace = true;
 	}
 	if (iRel->rd_isnailed)
 	{
 		if (!IsIgnoringSystemIndexes())
-			ereport(ERROR,
+			elog(ERROR,
-					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 "must be ignoring system indexes to reindex nailed index %u",
-				   errmsg("the target relation %u is nailed", indexId)));
+				 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,
+	bool		overwrite;
-				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
+	 * Should be ignoring system indexes if we are reindexing a system table.
-	 * reindex.
+	 * (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())
+		if (!IsIgnoringSystemIndexes())		/* shouldn't happen */
-		{
+			elog(ERROR,
-			overwrite = true;
+				 "must be ignoring system indexes to reindex shared table %u",
-			deactivate_needed = true;
+				 relid);
-		}
+		overwrite = true;
-		else
-			ereport(ERROR,
-					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-					 errmsg("the target relation %u is shared", relid)));
 	}
 
 	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,
+	ScanKeyEntryInitialize(&entry, 0,
-						   F_OIDEQ, ObjectIdGetDatum(relid));
+						   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;

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