From 8e18d04d4daf34b8a557e2dc553a7754b255cd9a Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Thu, 28 Nov 2013 20:57:20 -0500 Subject: [PATCH] Refine our definition of what constitutes a system relation. Although user-defined relations can't be directly created in pg_catalog, it's possible for them to end up there, because you can create them in some other schema and then use ALTER TABLE .. SET SCHEMA to move them there. Previously, such relations couldn't afterwards be manipulated, because IsSystemRelation()/IsSystemClass() rejected all attempts to modify objects in the pg_catalog schema, regardless of their origin. With this patch, they now reject only those objects in pg_catalog which were created at initdb-time, allowing most operations on user-created tables in pg_catalog to proceed normally. This patch also adds new functions IsCatalogRelation() and IsCatalogClass(), which is similar to IsSystemRelation() and IsSystemClass() but with a slightly narrower definition: only TOAST tables of system catalogs are included, rather than *all* TOAST tables. This is currently used only for making decisions about when invalidation messages need to be sent, but upcoming logical decoding patches will find other uses for this information. Andres Freund, with some modifications by me. --- src/backend/access/heap/heapam.c | 2 +- src/backend/catalog/aclchk.c | 2 +- src/backend/catalog/catalog.c | 70 +++++++++++++++++++---- src/backend/catalog/heap.c | 11 +++- src/backend/commands/cluster.c | 2 +- src/backend/commands/indexcmds.c | 5 +- src/backend/commands/tablecmds.c | 6 +- src/backend/commands/trigger.c | 2 +- src/backend/optimizer/util/plancat.c | 2 +- src/backend/rewrite/rewriteDefine.c | 2 +- src/backend/tcop/utility.c | 2 +- src/backend/utils/cache/inval.c | 8 +-- src/include/catalog/catalog.h | 4 +- src/test/regress/expected/alter_table.out | 34 +++++++++++ src/test/regress/sql/alter_table.sql | 28 +++++++++ 15 files changed, 149 insertions(+), 31 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index f35afddbea..31496a3063 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2465,7 +2465,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples, * because the heaptuples data structure is all in local memory, not in * the shared buffer. */ - if (IsSystemRelation(relation)) + if (IsCatalogRelation(relation)) { for (i = 0; i < ntuples; i++) CacheInvalidateHeapTuple(relation, heaptuples[i], NULL); diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 06aa76627a..5a46fd963a 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -3628,7 +3628,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid, * themselves. ACL_USAGE is if we ever have system sequences. */ if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) && - IsSystemClass(classForm) && + IsSystemClass(table_oid, classForm) && classForm->relkind != RELKIND_VIEW && !has_rolcatupdate(roleid) && !allowSystemTableMods) diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 577206cd26..7719798457 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -97,22 +97,20 @@ GetDatabasePath(Oid dbNode, Oid spcNode) /* * IsSystemRelation - * True iff the relation is a system catalog relation. + * 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. * * 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. - * - * We now just test if the relation is in the system catalog namespace; - * so it's no longer necessary to forbid user relations from having - * names starting with pg_. + * also check IsToastRelation or use IsCatalogRelation(). */ bool IsSystemRelation(Relation relation) { - return IsSystemNamespace(RelationGetNamespace(relation)) || - IsToastNamespace(RelationGetNamespace(relation)); + return IsSystemClass(RelationGetRelid(relation), relation->rd_rel); } /* @@ -122,12 +120,60 @@ IsSystemRelation(Relation relation) * search pg_class directly. */ bool -IsSystemClass(Form_pg_class reltuple) +IsSystemClass(Oid relid, Form_pg_class reltuple) { - Oid relnamespace = reltuple->relnamespace; + return IsToastClass(reltuple) || IsCatalogClass(relid, reltuple); +} - return IsSystemNamespace(relnamespace) || - IsToastNamespace(relnamespace); +/* + * 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. + * + * Note that IsSystemRelation() returns true for ALL toast relations, + * but this function returns true only for toast relations of system + * catalogs. + */ +bool +IsCatalogRelation(Relation relation) +{ + return IsCatalogClass(RelationGetRelid(relation), relation->rd_rel); +} + +/* + * IsCatalogClass + * True iff the relation is a system catalog relation. + * + * Check IsCatalogRelation() for details. + */ +bool +IsCatalogClass(Oid relid, Form_pg_class reltuple) +{ + 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. + * + * This test is safe since even a oid wraparound will preserve this + * property (c.f. GetNewObjectId()) and it has the advantage that it works + * correctly even if a user decides to create a relation in the pg_catalog + * namespace. + * ---- + */ + return relid < FirstNormalObjectId; } /* diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index a910f81666..6f2e1428eb 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -256,10 +256,17 @@ heap_create(const char *relname, Assert(OidIsValid(relid)); /* - * sanity checks + * Don't allow creating relations in pg_catalog directly, even though it + * is allowed to move user defined relations there. Semantics with search + * paths including pg_catalog are too confusing for now. + * + * But allow creating indexes on relations in pg_catalog even if + * allow_system_table_mods = off, upper layers already guarantee it's on a + * user defined relation, not a system one. */ if (!allow_system_table_mods && - (IsSystemNamespace(relnamespace) || IsToastNamespace(relnamespace)) && + ((IsSystemNamespace(relnamespace) && relkind != RELKIND_INDEX) || + IsToastNamespace(relnamespace)) && IsNormalProcessingMode()) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index f6a5bfe8d1..afc6a78650 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1354,7 +1354,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, * ones the dependency changes would change. It's too late to be * making any data changes to the target catalog. */ - if (IsSystemClass(relform1)) + if (IsSystemClass(r1, relform1)) elog(ERROR, "cannot swap toast files by links for system catalogs"); /* Delete old dependencies */ diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 2155252e4a..9b97e44867 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1824,6 +1824,7 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user) while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_class classtuple = (Form_pg_class) GETSTRUCT(tuple); + Oid relid = HeapTupleGetOid(tuple); if (classtuple->relkind != RELKIND_RELATION && classtuple->relkind != RELKIND_MATVIEW) @@ -1835,7 +1836,7 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user) continue; /* Check user/system classification, and optionally skip */ - if (IsSystemClass(classtuple)) + if (IsSystemClass(relid, classtuple)) { if (!do_system) continue; @@ -1850,7 +1851,7 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user) continue; /* got it already */ old = MemoryContextSwitchTo(private_context); - relids = lappend_oid(relids, HeapTupleGetOid(tuple)); + relids = lappend_oid(relids, relid); MemoryContextSwitchTo(old); } heap_endscan(scan); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3483107e59..1aa1ad9127 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -910,7 +910,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid, aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rel->relname); - if (!allowSystemTableMods && IsSystemClass(classform)) + if (!allowSystemTableMods && IsSystemClass(relOid, classform)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", @@ -2105,7 +2105,7 @@ renameatt_check(Oid myrelid, Form_pg_class classform, bool recursing) if (!pg_class_ownercheck(myrelid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, NameStr(classform->relname)); - if (!allowSystemTableMods && IsSystemClass(classform)) + if (!allowSystemTableMods && IsSystemClass(myrelid, classform)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", @@ -10872,7 +10872,7 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid, aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rv->relname); /* No system table modifications unless explicitly allowed. */ - if (!allowSystemTableMods && IsSystemClass(classform)) + if (!allowSystemTableMods && IsSystemClass(relid, classform)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index d86e9ad2c7..0008fc633b 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1174,7 +1174,7 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid, /* you must own the table to rename one of its triggers */ if (!pg_class_ownercheck(relid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rv->relname); - if (!allowSystemTableMods && IsSystemClass(form)) + if (!allowSystemTableMods && IsSystemClass(relid, form)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 954666ce04..de981cb372 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -128,7 +128,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, * Don't bother with indexes for an inheritance parent, either. */ if (inhparent || - (IgnoreSystemIndexes && IsSystemClass(relation->rd_rel))) + (IgnoreSystemIndexes && IsSystemRelation(relation))) hasindex = false; else hasindex = relation->rd_rel->relhasindex; diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c index 2eca531749..b30902d2cd 100644 --- a/src/backend/rewrite/rewriteDefine.c +++ b/src/backend/rewrite/rewriteDefine.c @@ -858,7 +858,7 @@ RangeVarCallbackForRenameRule(const RangeVar *rv, Oid relid, Oid oldrelid, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("\"%s\" is not a table or view", rv->relname))); - if (!allowSystemTableMods && IsSystemClass(form)) + if (!allowSystemTableMods && IsSystemClass(relid, form)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 5895102b51..7d75b3383f 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -110,7 +110,7 @@ CheckRelationOwnership(RangeVar *rel, bool noCatalogs) if (noCatalogs) { if (!allowSystemTableMods && - IsSystemClass((Form_pg_class) GETSTRUCT(tuple))) + IsSystemClass(relOid, (Form_pg_class) GETSTRUCT(tuple))) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied: \"%s\" is a system catalog", diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c index bfe7d787b7..997a62f8ba 100644 --- a/src/backend/utils/cache/inval.c +++ b/src/backend/utils/cache/inval.c @@ -1040,15 +1040,15 @@ CacheInvalidateHeapTuple(Relation relation, /* * We only need to worry about invalidation for tuples that are in system - * relations; user-relation tuples are never in catcaches and can't affect + * catalogs; user-relation tuples are never in catcaches and can't affect * the relcache either. */ - if (!IsSystemRelation(relation)) + if (!IsCatalogRelation(relation)) return; /* - * TOAST tuples can likewise be ignored here. Note that TOAST tables are - * considered system relations so they are not filtered by the above test. + * IsCatalogRelation() will return true for TOAST tables of system + * catalogs, but we don't care about those, either. */ if (IsToastRelation(relation)) return; diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h index 44b6f38743..493493f997 100644 --- a/src/include/catalog/catalog.h +++ b/src/include/catalog/catalog.h @@ -25,9 +25,11 @@ extern char *GetDatabasePath(Oid dbNode, Oid spcNode); extern bool IsSystemRelation(Relation relation); extern bool IsToastRelation(Relation relation); +extern bool IsCatalogRelation(Relation relation); -extern bool IsSystemClass(Form_pg_class reltuple); +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 IsToastNamespace(Oid namespaceId); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 7366392457..232a233be5 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2337,3 +2337,37 @@ FROM ( 0 | t (1 row) +-- Checks on creating and manipulation of user defined relations in +-- pg_catalog. +-- +-- XXX: It would be useful to add checks around trying to manipulate +-- catalog tables, but that might have ugly consequences when run +-- against an existing server with allow_system_table_mods = on. +SHOW allow_system_table_mods; + allow_system_table_mods +------------------------- + off +(1 row) + +-- disallowed because of search_path issues with pg_dump +CREATE TABLE pg_catalog.new_system_table(); +ERROR: permission denied to create "pg_catalog.new_system_table" +DETAIL: System catalog modifications are currently disallowed. +-- instead create in public first, move to catalog +CREATE TABLE new_system_table(id serial primary key, othercol text); +ALTER TABLE new_system_table SET SCHEMA pg_catalog; +-- XXX: it's currently impossible to move relations out of pg_catalog +ALTER TABLE new_system_table SET SCHEMA public; +ERROR: cannot remove dependency on schema pg_catalog because it is a system object +-- move back, will currently error out, already there +ALTER TABLE new_system_table SET SCHEMA pg_catalog; +ERROR: table new_system_table is already in schema "pg_catalog" +ALTER TABLE new_system_table RENAME TO old_system_table; +CREATE INDEX old_system_table__othercol ON old_system_table (othercol); +INSERT INTO old_system_table(othercol) VALUES ('somedata'), ('otherdata'); +UPDATE old_system_table SET id = -id; +DELETE FROM old_system_table WHERE othercol = 'somedata'; +TRUNCATE old_system_table; +ALTER TABLE old_system_table DROP CONSTRAINT new_system_table_pkey; +ALTER TABLE old_system_table DROP COLUMN othercol; +DROP TABLE old_system_table; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 0d3b79bfcf..b55543076f 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1567,3 +1567,31 @@ FROM ( FROM pg_class WHERE relkind IN ('r', 'i', 'S', 't', 'm') ) mapped; + +-- Checks on creating and manipulation of user defined relations in +-- pg_catalog. +-- +-- XXX: It would be useful to add checks around trying to manipulate +-- catalog tables, but that might have ugly consequences when run +-- against an existing server with allow_system_table_mods = on. + +SHOW allow_system_table_mods; +-- disallowed because of search_path issues with pg_dump +CREATE TABLE pg_catalog.new_system_table(); +-- instead create in public first, move to catalog +CREATE TABLE new_system_table(id serial primary key, othercol text); +ALTER TABLE new_system_table SET SCHEMA pg_catalog; + +-- XXX: it's currently impossible to move relations out of pg_catalog +ALTER TABLE new_system_table SET SCHEMA public; +-- move back, will currently error out, already there +ALTER TABLE new_system_table SET SCHEMA pg_catalog; +ALTER TABLE new_system_table RENAME TO old_system_table; +CREATE INDEX old_system_table__othercol ON old_system_table (othercol); +INSERT INTO old_system_table(othercol) VALUES ('somedata'), ('otherdata'); +UPDATE old_system_table SET id = -id; +DELETE FROM old_system_table WHERE othercol = 'somedata'; +TRUNCATE old_system_table; +ALTER TABLE old_system_table DROP CONSTRAINT new_system_table_pkey; +ALTER TABLE old_system_table DROP COLUMN othercol; +DROP TABLE old_system_table;