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.

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f35afddbea389168fce6d61bf35e35526151cc36..31496a3063e8e3ef0bd4de20de699ffdda893882 100644 (file)
--- 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 06aa76627abd77fda5a651c40d7ed3868531a24e..5a46fd963a158fad697b84d8d3cd9fd569280869 100644 (file)
--- 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 577206cd26be986e000f0f93393e37a4fe896417..7719798457919a936b1fbda6c3d7be7b7665d6ee 100644 (file)
--- 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);
+}
+
+/*
+ * 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);
+}
 
-   return IsSystemNamespace(relnamespace) ||
-       IsToastNamespace(relnamespace);
+/*
+ * 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 a910f81666cf838fe570b7902482ba8010fd22b9..6f2e1428ebe6fc317484aedb7ff25c8b02d25135 100644 (file)
--- 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 f6a5bfe8d15b523703ff1242d3d21431841b6592..afc6a7865081443958204ba1ef922b32fb90fc7e 100644 (file)
--- 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 2155252e4ad27563b56ccee5b56704ee5b2f47d6..9b97e44867a99584400746f35c650da6d25e46e0 100644 (file)
--- 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 3483107e595b11411b663bc7208ed4e98792dcd0..1aa1ad91277371649d7abba27694c3c710626a7d 100644 (file)
--- 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 d86e9ad2c7d099eb2b7b4a4626296e23b21f1b32..0008fc633babd41dec58bfa4594a6d5a0d57269d 100644 (file)
--- 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 954666ce04ce7b5fae849329ea1b67c34332878a..de981cb3727e78389904f8d413fd54e575226dd6 100644 (file)
--- 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 2eca531749aacb432019d0c1d0c886f6338519d5..b30902d2cdd7af34209ef92c7026e6924fe5ee1b 100644 (file)
--- 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 5895102b518460490ebbc8be7bce723c06249bde..7d75b3383fa9f2f7a3e3d2521ad610da57c95b7a 100644 (file)
--- 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 bfe7d787b7b4ce8d45689bf8c71efe3bdf7d8f0d..997a62f8bad249fb98366ab9be075a31748a431b 100644 (file)
--- 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 44b6f38743681a0706047d7a4b973ce00dfb0fd8..493493f99766a700381b7eb030b872ae6e6282e7 100644 (file)
--- 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 7366392457ec40173ed5afecd9684f8b92fd7d0b..232a233be54834c93c8be8f8cdf0efb37421c32d 100644 (file)
--- 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 0d3b79bfcfd26531e6cbc59411024f295fc18154..b55543076f9ac68db07fc0df447f3bcda4511dcb 100644 (file)
--- 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;