Clean up the behavior and API of catalog.c's is-catalog-relation tests.

tglsfdc · tglsfdc · commit 2d7d946cd323 · 2019-05-08T23:27:38.000-04:00
The right way for IsCatalogRelation/Class to behave is to return true for OIDs less than FirstBootstrapObjectId (not FirstNormalObjectId), without any of the ad-hoc fooling around with schema membership. The previous code was wrong because (1) it claimed that information_schema tables were not catalog relations but their toast tables were, which is silly; and (2) if you dropped and recreated information_schema, which is a supported operation, the behavior changed. That's even sillier. With this definition, "catalog relations" are exactly the ones traceable to the postgres.bki data, which seems like what we want. With this simplification, we don't actually need access to the pg_class tuple to identify a catalog relation; we only need its OID. Hence, replace IsCatalogClass with "IsCatalogRelationOid(oid)". But keep IsCatalogRelation as a convenience function. This allows fixing some arguably-wrong semantics in contrib/sepgsql and ReindexRelationConcurrently, which were using an IsSystemNamespace test where what they really should be using is IsCatalogRelationOid. The previous coding failed to protect toast tables of system catalogs, and also was not on board with the general principle that user-created tables do not become catalogs just by virtue of being renamed into pg_catalog. We can also get rid of a messy hack in ReindexMultipleTables. While we're at it, also rename IsSystemNamespace to IsCatalogNamespace, because the previous name invited confusion with the more expansive semantics used by IsSystemRelation/Class. Also improve the comments in catalog.c. There are a few remaining places in replication-related code that are special-casing OIDs below FirstNormalObjectId. I'm inclined to think those are wrong too, and if there should be any special case it should just extend to FirstBootstrapObjectId. But first we need to debate whether a FOR ALL TABLES publication should include information_schema. Discussion: https://postgr.es/m/21697.1557092753@sss.pgh.pa.us Discussion: https://postgr.es/m/15150.1557257111@sss.pgh.pa.us
diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
@@ -161,12 +161,10 @@ check_relation_privileges(Oid relOid,
 	 */
 	if (sepgsql_getenforce() > 0)
 	{
-		Oid			relnamespace = get_rel_namespace(relOid);
-
-		if (IsSystemNamespace(relnamespace) &&
-			(required & (SEPG_DB_TABLE__UPDATE |
+		if ((required & (SEPG_DB_TABLE__UPDATE |
 						 SEPG_DB_TABLE__INSERT |
-						 SEPG_DB_TABLE__DELETE)) != 0)
+						 SEPG_DB_TABLE__DELETE)) != 0 &&
+			IsCatalogRelationOid(relOid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("SELinux: hardwired security policy violation")));
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
@@ -520,7 +520,7 @@ GetNewObjectId(void)
 	 * Check for wraparound of the OID counter.  We *must* not return 0
 	 * (InvalidOid), and in normal operation we mustn't return anything below
 	 * FirstNormalObjectId since that range is reserved for initdb (see
-	 * IsCatalogClass()).  Note we are relying on unsigned comparison.
+	 * IsCatalogRelationOid()).  Note we are relying on unsigned comparison.
 	 *
 	 * During initdb, we start the OID generator at FirstBootstrapObjectId, so
 	 * we only wrap if before that point when in bootstrap or standalone mode.
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
@@ -53,15 +53,18 @@
 
 /*
  * IsSystemRelation
- *		True iff the relation is either a system catalog or toast table.
- *		By a system catalog, we mean one that created in the pg_catalog schema
- *		during initdb.  User-created relations in pg_catalog don't count as
- *		system catalogs.
+ *		True iff the relation is either a system catalog or a toast table.
+ *		See IsCatalogRelation for the exact definition of a system catalog.
  *
- *		NB: TOAST relations are considered system relations by this test
- *		for compatibility with the old IsSystemRelationName function.
- *		This is appropriate in many places but not all.  Where it's not,
- *		also check IsToastRelation or use IsCatalogRelation().
+ *		We treat toast tables of user relations as "system relations" for
+ *		protection purposes, e.g. you can't change their schemas without
+ *		special permissions.  Therefore, most uses of this function are
+ *		checking whether allow_system_table_mods restrictions apply.
+ *		For other purposes, consider whether you shouldn't be using
+ *		IsCatalogRelation instead.
+ *
+ *		This function does not perform any catalog accesses.
+ *		Some callers rely on that!
  */
 bool
 IsSystemRelation(Relation relation)
@@ -78,67 +81,74 @@ IsSystemRelation(Relation relation)
 bool
 IsSystemClass(Oid relid, Form_pg_class reltuple)
 {
-	return IsToastClass(reltuple) || IsCatalogClass(relid, reltuple);
+	/* IsCatalogRelationOid is a bit faster, so test that first */
+	return (IsCatalogRelationOid(relid) || IsToastClass(reltuple));
 }
 
 /*
  * IsCatalogRelation
- *		True iff the relation is a system catalog, or the toast table for
- *		a system catalog.  By a system catalog, we mean one that created
- *		in the pg_catalog schema during initdb.  As with IsSystemRelation(),
- *		user-created relations in pg_catalog don't count as system catalogs.
+ *		True iff the relation is a system catalog.
+ *
+ *		By a system catalog, we mean one that is created during the bootstrap
+ *		phase of initdb.  That includes not just the catalogs per se, but
+ *		also their indexes, and TOAST tables and indexes if any.
  *
- *		Note that IsSystemRelation() returns true for ALL toast relations,
- *		but this function returns true only for toast relations of system
- *		catalogs.
+ *		This function does not perform any catalog accesses.
+ *		Some callers rely on that!
  */
 bool
 IsCatalogRelation(Relation relation)
 {
-	return IsCatalogClass(RelationGetRelid(relation), relation->rd_rel);
+	return IsCatalogRelationOid(RelationGetRelid(relation));
 }
 
 /*
- * IsCatalogClass
- *		True iff the relation is a system catalog relation.
+ * IsCatalogRelationOid
+ *		True iff the relation identified by this OID is a system catalog.
+ *
+ *		By a system catalog, we mean one that is created during the bootstrap
+ *		phase of initdb.  That includes not just the catalogs per se, but
+ *		also their indexes, and TOAST tables and indexes if any.
  *
- * Check IsCatalogRelation() for details.
+ *		This function does not perform any catalog accesses.
+ *		Some callers rely on that!
  */
 bool
-IsCatalogClass(Oid relid, Form_pg_class reltuple)
+IsCatalogRelationOid(Oid relid)
 {
-	Oid			relnamespace = reltuple->relnamespace;
-
 	/*
-	 * Never consider relations outside pg_catalog/pg_toast to be catalog
-	 * relations.
-	 */
-	if (!IsSystemNamespace(relnamespace) && !IsToastNamespace(relnamespace))
-		return false;
-
-	/* ----
-	 * Check whether the oid was assigned during initdb, when creating the
-	 * initial template database. Minus the relations in information_schema
-	 * excluded above, these are integral part of the system.
-	 * We could instead check whether the relation is pinned in pg_depend, but
-	 * this is noticeably cheaper and doesn't require catalog access.
+	 * We consider a relation to be a system catalog if it has an OID that was
+	 * manually assigned or assigned by genbki.pl.  This includes all the
+	 * defined catalogs, their indexes, and their TOAST tables and indexes.
 	 *
-	 * This test is safe since even an oid wraparound will preserve this
-	 * property (cf. GetNewObjectId()) and it has the advantage that it works
-	 * correctly even if a user decides to create a relation in the pg_catalog
-	 * namespace.
-	 * ----
+	 * This rule excludes the relations in information_schema, which are not
+	 * integral to the system and can be treated the same as user relations.
+	 * (Since it's valid to drop and recreate information_schema, any rule
+	 * that did not act this way would be wrong.)
+	 *
+	 * This test is reliable since an OID wraparound will skip this range of
+	 * OIDs; see GetNewObjectId().
 	 */
-	return relid < FirstNormalObjectId;
+	return (relid < (Oid) FirstBootstrapObjectId);
 }
 
 /*
  * IsToastRelation
  *		True iff relation is a TOAST support relation (or index).
+ *
+ *		Does not perform any catalog accesses.
  */
 bool
 IsToastRelation(Relation relation)
 {
+	/*
+	 * What we actually check is whether the relation belongs to a pg_toast
+	 * namespace.  This should be equivalent because of restrictions that are
+	 * enforced elsewhere against creating user relations in, or moving
+	 * relations into/out of, a pg_toast namespace.  Notice also that this
+	 * will not say "true" for toast tables belonging to other sessions' temp
+	 * tables; we expect that other mechanisms will prevent access to those.
+	 */
 	return IsToastNamespace(RelationGetNamespace(relation));
 }
 
@@ -157,14 +167,16 @@ IsToastClass(Form_pg_class reltuple)
 }
 
 /*
- * IsSystemNamespace
+ * IsCatalogNamespace
  *		True iff namespace is pg_catalog.
  *
+ *		Does not perform any catalog accesses.
+ *
  * NOTE: the reason this isn't a macro is to avoid having to include
  * catalog/pg_namespace.h in a lot of places.
  */
 bool
-IsSystemNamespace(Oid namespaceId)
+IsCatalogNamespace(Oid namespaceId)
 {
 	return namespaceId == PG_CATALOG_NAMESPACE;
 }
@@ -173,9 +185,13 @@ IsSystemNamespace(Oid namespaceId)
  * IsToastNamespace
  *		True iff namespace is pg_toast or my temporary-toast-table namespace.
  *
+ *		Does not perform any catalog accesses.
+ *
  * Note: this will return false for temporary-toast-table namespaces belonging
  * to other backends.  Those are treated the same as other backends' regular
  * temp table namespaces, and access is prevented where appropriate.
+ * If you need to check for those, you may be able to use isAnyTempNamespace,
+ * but beware that that does involve a catalog access.
  */
 bool
 IsToastNamespace(Oid namespaceId)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
@@ -324,7 +324,7 @@ heap_create(const char *relname,
 	 * user defined relation, not a system one.
 	 */
 	if (!allow_system_table_mods &&
-		((IsSystemNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
+		((IsCatalogNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
 		 IsToastNamespace(relnamespace)) &&
 		IsNormalProcessingMode())
 		ereport(ERROR,
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
@@ -93,14 +93,16 @@ check_publication_add_relation(Relation targetrel)
  *
  * Note this also excludes all tables with relid < FirstNormalObjectId,
  * ie all tables created during initdb.  This mainly affects the preinstalled
- * information_schema.  (IsCatalogClass() only checks for these inside
- * pg_catalog and toast schemas.)
+ * information_schema.  (IsCatalogRelationOid() only excludes tables with
+ * relid < FirstBootstrapObjectId, making that test rather redundant, but
+ * really we should get rid of the FirstNormalObjectId test not
+ * IsCatalogRelationOid.)
  */
 static bool
 is_publishable_class(Oid relid, Form_pg_class reltuple)
 {
 	return reltuple->relkind == RELKIND_RELATION &&
-		!IsCatalogClass(relid, reltuple) &&
+		!IsCatalogRelationOid(relid) &&
 		reltuple->relpersistence == RELPERSISTENCE_PERMANENT &&
 		relid >= FirstNormalObjectId;
 }
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
@@ -2590,15 +2590,11 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 			continue;
 
 		/*
-		 * Skip system tables that index_create() would reject to index
-		 * concurrently.  XXX We need the additional check for
-		 * FirstNormalObjectId to skip information_schema tables, because
-		 * IsCatalogClass() here does not cover information_schema, but the
-		 * check in index_create() will error on the TOAST tables of
-		 * information_schema tables.
+		 * Skip system tables, since index_create() would reject indexing them
+		 * concurrently (and it would likely fail if we tried).
 		 */
 		if (concurrent &&
-			(IsCatalogClass(relid, classtuple) || relid < FirstNormalObjectId))
+			IsCatalogRelationOid(relid))
 		{
 			if (!concurrent_warning)
 				ereport(WARNING,
@@ -2842,7 +2838,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 							 errmsg("concurrent reindex is not supported for shared relations")));
 
 				/* A system catalog cannot be reindexed concurrently */
-				if (IsSystemNamespace(get_rel_namespace(heapId)))
+				if (IsCatalogRelationOid(heapId))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("concurrent reindex is not supported for catalog relations")));
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -12453,9 +12453,10 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
 		 * Also, explicitly avoid any shared tables, temp tables, or TOAST
 		 * (TOAST will be moved with the main table).
 		 */
-		if (IsSystemNamespace(relForm->relnamespace) || relForm->relisshared ||
+		if (IsCatalogNamespace(relForm->relnamespace) ||
+			relForm->relisshared ||
 			isAnyTempNamespace(relForm->relnamespace) ||
-			relForm->relnamespace == PG_TOAST_NAMESPACE)
+			IsToastNamespace(relForm->relnamespace))
 			continue;
 
 		/* Only move the object type requested */
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
@@ -3316,8 +3316,8 @@ RelationBuildLocalRelation(const char *relname,
 	else
 		rel->rd_rel->relispopulated = true;
 
-	/* system relations and non-table objects don't have one */
-	if (!IsSystemNamespace(relnamespace) &&
+	/* set replica identity -- system catalogs and non-tables don't have one */
+	if (!IsCatalogNamespace(relnamespace) &&
 		(relkind == RELKIND_RELATION ||
 		 relkind == RELKIND_MATVIEW ||
 		 relkind == RELKIND_PARTITIONED_TABLE))
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
@@ -24,9 +24,10 @@ extern bool IsCatalogRelation(Relation relation);
 
 extern bool IsSystemClass(Oid relid, Form_pg_class reltuple);
 extern bool IsToastClass(Form_pg_class reltuple);
-extern bool IsCatalogClass(Oid relid, Form_pg_class reltuple);
 
-extern bool IsSystemNamespace(Oid namespaceId);
+extern bool IsCatalogRelationOid(Oid relid);
+
+extern bool IsCatalogNamespace(Oid namespaceId);
 extern bool IsToastNamespace(Oid namespaceId);
 
 extern bool IsReservedName(const char *name);

Original file line number	Diff line number	Diff line change
`@@ -520,7 +520,7 @@ GetNewObjectId(void)`
`520`	`520`	`* Check for wraparound of the OID counter. We must not return 0`
`521`	`521`	`* (InvalidOid), and in normal operation we mustn't return anything below`
`522`	`522`	`* FirstNormalObjectId since that range is reserved for initdb (see`
`523`		`- * IsCatalogClass()). Note we are relying on unsigned comparison.`
	`523`	`+ * IsCatalogRelationOid()). Note we are relying on unsigned comparison.`
`524`	`524`	`*`
`525`	`525`	`* During initdb, we start the OID generator at FirstBootstrapObjectId, so`
`526`	`526`	`* we only wrap if before that point when in bootstrap or standalone mode.`