[PATCH] Cleanup: unify checks for catalog modification
Started by Marti Raudseppabout 11 years ago3 messages
Hi list,
I happened to notice that there are no less than 14 places in the code
that check whether a relation is a system catalog and throwing the
error "permission denied: "foo" is a system catalog"
The attached patch factors all of those into a single
ForbidSystemTableMods() function. Is this considered an improvement?
Should I add it to CommitFest?
Regards,
Marti
Attachments:
0001-Cleanup-unify-checks-for-catalog-modification.patchbinary/octet-stream; name=0001-Cleanup-unify-checks-for-catalog-modification.patchDownload
From 79dfa24649bbbaa552f8c00679f51a8f8eadb129 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Thu, 2 Oct 2014 23:24:01 +0300
Subject: [PATCH] Cleanup: unify checks for catalog modification
---
src/backend/catalog/catalog.c | 15 ++++++++++++
src/backend/commands/policy.c | 14 ++---------
src/backend/commands/tablecmds.c | 49 ++++++-------------------------------
src/backend/commands/trigger.c | 18 +++-----------
src/backend/rewrite/rewriteDefine.c | 12 ++-------
src/include/catalog/catalog.h | 1 +
6 files changed, 30 insertions(+), 79 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 2eb2c2f..db15d8e 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -92,6 +92,21 @@ IsCatalogRelation(Relation relation)
}
/*
+ * ForbidSystemTableMods
+ * Emit error when referenced class is a system catalog and catalog
+ * modifications (allow_system_table_mods) are not allowed.
+ */
+void
+ForbidSystemTableMods(Oid relid, Form_pg_class reltuple)
+{
+ if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a system catalog",
+ NameStr(reltuple->relname))));
+}
+
+/*
* IsCatalogClass
* True iff the relation is a system catalog relation.
*
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 3627539..00ad109 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -76,13 +76,7 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
/* Must own relation. */
if (!pg_class_ownercheck(relid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rv->relname);
-
- /* No system table modifications unless explicitly allowed. */
- if (!allowSystemTableMods && IsSystemClass(relid, classform))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- rv->relname)));
+ ForbidSystemTableMods(relid, classform);
/* Relation type MUST be a table. */
if (relkind != RELKIND_RELATION)
@@ -409,11 +403,7 @@ RemovePolicyById(Oid policy_id)
errmsg("\"%s\" is not a table",
RelationGetRelationName(rel))));
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ ForbidSystemTableMods(relid, rel->rd_rel);
simple_heap_delete(pg_rowsecurity_rel, &tuple->t_self);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ecdff1e..1b6086d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -941,12 +941,7 @@ RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
!pg_namespace_ownercheck(classform->relnamespace, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
rel->relname);
-
- if (!allowSystemTableMods && IsSystemClass(relOid, classform))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- rel->relname)));
+ ForbidSystemTableMods(relOid, classform);
ReleaseSysCache(tuple);
@@ -1279,12 +1274,7 @@ truncate_check_rel(Relation rel)
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_CLASS,
RelationGetRelationName(rel));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ ForbidSystemTableMods(rel->rd_id, rel->rd_rel);
/*
* Don't allow truncate on temp tables of other backends ... their local
@@ -2146,11 +2136,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(myrelid, classform))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- NameStr(classform->relname))));
+ ForbidSystemTableMods(myrelid, classform);
}
/*
@@ -4206,12 +4192,7 @@ ATSimplePermissions(Relation rel, int allowed_targets)
if (!pg_class_ownercheck(RelationGetRelid(rel), GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
RelationGetRelationName(rel));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ ForbidSystemTableMods(rel->rd_id, rel->rd_rel);
}
/*
@@ -6037,11 +6018,7 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
errmsg("referenced relation \"%s\" is not a table",
RelationGetRelationName(pkrel))));
- if (!allowSystemTableMods && IsSystemRelation(pkrel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(pkrel))));
+ ForbidSystemTableMods(pkrel->rd_id, pkrel->rd_rel);
/*
* References from permanent or unlogged tables to temp tables, and from
@@ -11481,13 +11458,7 @@ RangeVarCallbackOwnsRelation(const RangeVar *relation,
if (!pg_class_ownercheck(relId, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
relation->relname);
-
- if (!allowSystemTableMods &&
- IsSystemClass(relId, (Form_pg_class) GETSTRUCT(tuple)))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- relation->relname)));
+ ForbidSystemTableMods(relId, (Form_pg_class) GETSTRUCT(tuple));
ReleaseSysCache(tuple);
}
@@ -11516,13 +11487,7 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
/* Must own relation. */
if (!pg_class_ownercheck(relid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, rv->relname);
-
- /* No system table modifications unless explicitly allowed. */
- if (!allowSystemTableMods && IsSystemClass(relid, classform))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- rv->relname)));
+ ForbidSystemTableMods(relid, classform);
/*
* Extract the specified relation type from the statement parse tree.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index f4c0ffa..43bdd32 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -226,11 +226,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
errmsg("\"%s\" is not a table or view",
RelationGetRelationName(rel))));
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ ForbidSystemTableMods(rel->rd_id, rel->rd_rel);
if (stmt->isconstraint)
{
@@ -1112,11 +1108,7 @@ RemoveTriggerById(Oid trigOid)
errmsg("\"%s\" is not a table, view, or foreign table",
RelationGetRelationName(rel))));
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ ForbidSystemTableMods(relid, rel->rd_rel);
/*
* Delete the pg_trigger tuple.
@@ -1220,11 +1212,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(relid, form))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- rv->relname)));
+ ForbidSystemTableMods(relid, form);
ReleaseSysCache(tuple);
}
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 660d069..8ea51d8 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -265,11 +265,7 @@ DefineQueryRewrite(char *rulename,
errmsg("\"%s\" is not a table or view",
RelationGetRelationName(event_relation))));
- if (!allowSystemTableMods && IsSystemRelation(event_relation))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(event_relation))));
+ ForbidSystemTableMods(event_relid, event_relation->rd_rel);
/*
* Check user has permission to apply rules to this relation.
@@ -881,11 +877,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(relid, form))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- rv->relname)));
+ ForbidSystemTableMods(relid, form);
/* you must own the table to rename one of its rules */
if (!pg_class_ownercheck(relid, GetUserId()))
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
index 9d63ac2..553ceb2 100644
--- a/src/include/catalog/catalog.h
+++ b/src/include/catalog/catalog.h
@@ -34,6 +34,7 @@ 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 void ForbidSystemTableMods(Oid relid, Form_pg_class reltuple);
extern bool IsSystemNamespace(Oid namespaceId);
extern bool IsToastNamespace(Oid namespaceId);
--
2.1.2
Re: [PATCH] Cleanup: unify checks for catalog modification
Marti Raudsepp <marti@juffo.org> writes:
I happened to notice that there are no less than 14 places in the code
that check whether a relation is a system catalog and throwing the
error "permission denied: "foo" is a system catalog"
The attached patch factors all of those into a single
ForbidSystemTableMods() function. Is this considered an improvement?
I'd argue not. The code bulk savings is minimal, and this change
would degrade the usefulness of the file/line number reporting that's
built into ereport(). Admittedly it's a judgment call --- we've certainly
built error-reporting subroutines in cases where a significant amount of
complexity could be folded into the subroutine. But I don't think this
case meets the threshold.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [PATCH] Cleanup: unify checks for catalog modification
Tom Lane wrote:
Marti Raudsepp <marti@juffo.org> writes:
I happened to notice that there are no less than 14 places in the code
that check whether a relation is a system catalog and throwing the
error "permission denied: "foo" is a system catalog"The attached patch factors all of those into a single
ForbidSystemTableMods() function. Is this considered an improvement?I'd argue not. The code bulk savings is minimal, and this change
would degrade the usefulness of the file/line number reporting that's
built into ereport().
Along these lines, I've sometimes thought that it could be useful to
pass down file/line info from certain callers down to certain generic
check subroutines such as the one being proposed here. (I can't recall
any specific examples offhand.) Of course, doing it manually would be
very tedious and error prone, but perhaps we could have something like a
macro system that both sets up arguments in the called function, and
sets up the values in the callee.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers