On 2013-11-12 19:24:39 +0100, Andres Freund wrote:
> On 2013-11-12 13:18:19 -0500, Robert Haas wrote:
> > On Tue, Nov 12, 2013 at 12:50 PM, Andres Freund <and...@2ndquadrant.com>
> > wrote:
> > > Completely agreed. As evidenced by the fact that the current change
> > > doesn't update all relevant comments & code. I wonder if we shouldn't
> > > leave the function the current way and just add a new function for the
> > > new behaviour.
> > > The hard thing with that would be coming up with a new
> > > name. IsSystemRelationId() having a different behaviour than
> > > IsSystemRelation() seems strange to me, so just keeping that and
> > > adapting the callers seems wrong to me.
> > > IsInternalRelation()? IsCatalogRelation()?
> >
> > Well, I went through and looked at the places that were affected by
> > this and I tend to think that most places will be happier with the new
> > definition.
>
> I agree that many if not most want the new definition.
>
> > If there are call sites that want the existing test, maybe we should
> > have IsRelationInSystemNamespace() for that, and reserve
> > IsSystemRelation() for the test as to whether it's a bona fide system
> > catalog.
>
> The big reason that I think we do not want the new behaviour for all is:
>
> * 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.
>
> the current state of things would allow to modify toast relations in
> some places :/
So, I think I found a useful defintion of IsSystemRelation() that fixes
many of the issues with moving relations to pg_catalog: Continue to
treat all pg_toast.* relations as system tables, but only consider
initdb created relations in pg_class.
I've then added IsCatalogRelation() which has a narrower definition of
system relations, namely, it only counts toast tables if they are a
catalog's toast table.
This allows far more actions on user defined relations moved to
pg_catalog. Now they aren't stuck there anymore and can be renamed,
dropped et al. With one curious exception: We still cannot move a
relation out of pg_catalog.
I've included a hunk to allow creation of indexes on relations in
pg_catalog in heap_create(), indexes on catalog relations are prevented
way above, but maybe that should rather be a separate commit.
What do you think?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 91a22bd7fb998b609e73a69b941999596ce4569f Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 13 Nov 2013 16:15:16 +0100
Subject: [PATCH] Don't regard user defined relations in pg_catalog as system
tables.
This allows for a more consistent behaviour of user defined relations
in pg_catalog. As before it's not possible to create relations in
pg_catalog, but now they can, after being moved there, manipulated
normally. Before it was impossible to move or drop user defined
relations in pg_catalog, now that's allowed.
To that end, modify IsSystemRelation/Class() to not regard user
defined tables in pg_catalog as system relations and add
IsCatalogRelation/Class() which don't regard toast relations as
catalog relation unless they are a catalog relation's toast table.
This is also preparation for logical decoding which needs
IsCatalogRelation().
---
src/backend/access/heap/heapam.c | 2 +-
src/backend/catalog/aclchk.c | 2 +-
src/backend/catalog/catalog.c | 69 ++++++++++++++++++++++++++-----
src/backend/catalog/heap.c | 11 ++++-
src/backend/commands/cluster.c | 2 +-
src/backend/commands/indexcmds.c | 5 ++-
src/backend/commands/tablecmds.c | 8 ++--
src/backend/commands/trigger.c | 2 +-
src/backend/optimizer/util/plancat.c | 2 +-
src/backend/rewrite/rewriteDefine.c | 4 +-
src/backend/tcop/utility.c | 2 +-
src/include/catalog/catalog.h | 4 +-
src/test/regress/expected/alter_table.out | 34 +++++++++++++++
src/test/regress/sql/alter_table.sql | 28 +++++++++++++
14 files changed, 148 insertions(+), 27 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a21f31b..bbb71c7 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 06aa766..5a46fd9 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 577206c..885ba27 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -102,17 +102,18 @@ GetDatabasePath(Oid dbNode, Oid spcNode)
* 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.
+ * also check IsToastRelation or use IsCatalogRelation().
*
- * 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_.
+ * We now just test if the relation is either in the toast namespace, or
+ * in the system namespace and created during initdb. This way it's no
+ * longer necessary to forbid user relations from having names starting
+ * with pg_ and it allows manipulating user defined relations in
+ * pg_catalog.
*/
bool
IsSystemRelation(Relation relation)
{
- return IsSystemNamespace(RelationGetNamespace(relation)) ||
- IsToastNamespace(RelationGetNamespace(relation));
+ return IsSystemClass(RelationGetRelid(relation), relation->rd_rel);
}
/*
@@ -122,12 +123,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;
+ if (IsToastClass(reltuple))
+ return true;
+ return IsCatalogClass(relid, reltuple);
+}
+
+/*
+ * IsCatalogRelation
+ * True iff the relation is a system catalog relation, i.e. a relation
+ * created in pg_catalog by initdb and their corresponding toast
+ * relations.
+ *
+ * In contrast to IsSystemRelation() toast relations are *not* included
+ * in this set unless they are a catalog relation's toast table.
+ */
+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 a910f81..6f2e142 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 f6a5bfe..afc6a78 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 2155252..9b97e44 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 0b31f55..92dd505 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",
@@ -1248,7 +1248,7 @@ truncate_check_rel(Relation rel)
aclcheck_error(aclresult, ACL_KIND_CLASS,
RelationGetRelationName(rel));
- if (!allowSystemTableMods && IsSystemRelation(rel))
+ if (!allowSystemTableMods && IsCatalogRelation(rel))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
@@ -2104,7 +2104,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",
@@ -10870,7 +10870,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 d86e9ad..0008fc6 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 954666c..de981cb 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 2eca531..3641d66 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -265,7 +265,7 @@ DefineQueryRewrite(char *rulename,
errmsg("\"%s\" is not a table or view",
RelationGetRelationName(event_relation))));
- if (!allowSystemTableMods && IsSystemRelation(event_relation))
+ if (!allowSystemTableMods && IsCatalogRelation(event_relation))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
@@ -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 6a7bf0d..939d761 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/include/catalog/catalog.h b/src/include/catalog/catalog.h
index 44b6f38..493493f 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 7366392..232a233 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 0d3b79b..b555430 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;
--
1.8.5.rc1.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
