Alvaro Herrera wrote:
> I didn't check the rest of the code, so don't count this as a review.
I had a look at aclchk.c and didn't like your change to
objectNamesToOids; seems rather baroque. I changed it per the attached
patch.
Moreover I didn't very much like the way aclcheck_error_col is dealing
with two or one % escapes. I think you should have a separate routine
for the column case, and prepend a dummy string to no_priv_msg.
Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to
check whether col_privs is NIL?
Is there enough common code in ExecGrant_Relation to justify the way you
have it? Can the common be refactored in a better way that separates
the two cases more clearly?
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
diff -u src/backend/catalog/aclchk.c src/backend/catalog/aclchk.c
--- src/backend/catalog/aclchk.c 14 Nov 2008 12:24:15 -0000
+++ src/backend/catalog/aclchk.c 14 Nov 2008 20:46:06 -0000
@@ -55,7 +55,8 @@
static void ExecGrant_Namespace(InternalGrant *grantStmt);
static void ExecGrant_Tablespace(InternalGrant *grantStmt);
-static List *objectNamesToOids(GrantObjectType objtype, List *objnames, Oid in_relOid);
+static List *objectNamesToOids(GrantObjectType objtype, List *objnames);
+static List *columnNamesToAttnums(List *colnames, Oid relid);
static AclMode string_to_privilege(const char *privname);
static const char *privilege_to_string(AclMode privilege);
static AclMode restrict_and_check_grant(bool is_grant, AclMode avail_goptions,
@@ -264,7 +265,7 @@
*/
istmt.is_grant = stmt->is_grant;
istmt.objtype = stmt->objtype;
- istmt.objects = objectNamesToOids(stmt->objtype, stmt->objects, 0);
+ istmt.objects = objectNamesToOids(stmt->objtype, stmt->objects);
/* all_privs to be filled below */
/* privileges to be filled below */
istmt.col_privs = NIL;
@@ -402,23 +403,24 @@
foreach(cell_obj, istmt.objects)
{
Oid relOid = lfirst_oid(cell_obj);
- List *cols_oids = NIL;
+ List *colnums;
ListCell *cell_coloid;
/* Get the attribute numbers for this relation for the columns affected */
- cols_oids = objectNamesToOids(ACL_OBJECT_COLUMN, privnode->cols, relOid);
+ colnums = columnNamesToAttnums(privnode->cols, relOid);
/* Loop through the columns listed for this privilege and
* add them to the col_privs list of privileges */
- foreach(cell_coloid, cols_oids)
+ foreach(cell_coloid, colnums)
{
- ListCell *cell_colprivs;
- int found = false;
- AttrNumber curr_attnum = lfirst_int(cell_coloid);
+ AttrNumber curr_attnum = lfirst_int(cell_coloid);
+ ListCell *cell_colprivs;
+ int found = false;
/* Check if we have already seen this column, and if
* so then just add to its aclmask. */
- foreach(cell_colprivs, istmt.col_privs) {
+ foreach(cell_colprivs, istmt.col_privs)
+ {
ColPrivs *col_privs = (ColPrivs *) lfirst(cell_colprivs);
if (col_privs->relOid == relOid && col_privs->attnum == curr_attnum)
@@ -487,50 +489,18 @@
* objectNamesToOids
*
* Turn a list of object names of a given type into an Oid list.
- * For columns, turn a list of column names into a list of
- * attribute numbers, for a given relation in_relOid.
*/
static List *
-objectNamesToOids(GrantObjectType objtype, List *objnames, Oid in_relOid)
+objectNamesToOids(GrantObjectType objtype, List *objnames)
{
List *objects = NIL;
ListCell *cell;
Assert(objnames != NIL);
+ AssertArg(objtype != ACL_OBJECT_COLUMN);
switch (objtype)
{
- case ACL_OBJECT_COLUMN:
- foreach(cell, objnames)
- {
- AttrNumber attnum;
-
- attnum = get_attnum(in_relOid,strVal(lfirst(cell)));
- if (attnum == InvalidAttrNumber)
- {
- HeapTuple tuple;
- Form_pg_class pg_class_tuple;
-
- tuple = SearchSysCache(RELOID,
- ObjectIdGetDatum(in_relOid),
- 0, 0, 0);
-
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", in_relOid);
-
- pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
-
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_COLUMN),
- errmsg("column \"%s\" of relation %s does not exist.",
- strVal(lfirst(cell)), NameStr(pg_class_tuple->relname))));
-
- ReleaseSysCache(tuple);
- }
-
- objects = lappend_int(objects, attnum);
- }
- break;
case ACL_OBJECT_RELATION:
case ACL_OBJECT_SEQUENCE:
foreach(cell, objnames)
@@ -647,6 +617,39 @@
}
/*
+ * columnNamesToAttnums
+ *
+ * Turn a list of column names into a list of attribute numbers, for the given
+ * relation.
+ */
+static List *
+columnNamesToAttnums(List *colnames, Oid relid)
+{
+ ListCell *cell;
+ List *colnums = NIL;
+
+ foreach(cell, colnames)
+ {
+ AttrNumber attnum;
+
+ attnum = get_attnum(relid, strVal(lfirst(cell)));
+ if (attnum == InvalidAttrNumber)
+ {
+ Relation rel = relation_open(relid, AccessShareLock);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ strVal(lfirst(cell)), RelationGetRelationName(rel))));
+ }
+
+ colnums = lappend_int(colnums, attnum);
+ }
+
+ return colnums;
+}
+
+/*
* This processes both sequences and non-sequences.
*/
static void
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
