Re: [PATCH] DefaultACLs

From: Joshua Tolley <eggyknap(at)gmail(dot)com>
To: Petr Jelinek <pjmodos(at)pjmodos(dot)net>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] DefaultACLs
Date: 2009-07-18 01:46:44
Message-ID: 20090718014644.GG12076@eddie
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Jul 14, 2009 at 11:10:00PM +0200, Petr Jelinek wrote:
> Hello,
>
> this is first public version of our DefaultACLs patch as described on
> http://wiki.postgresql.org/wiki/DefaultACL .

Ok, here's my first crack at a comprehensive review. There's more I need to
look at, eventually. Some of these are very minor stylistic comments, and some
are probably just because I've much less of a clue, in general, than I'd like
to think I have.

First, as you've already pointed out, this needs documentation.

Once I added the missing semicolon mentioned earlier, it applies and builds
fine. The regression tests, however, seem to assume that they'll be run as the
postgres user, and the privileges test failed. Here's part of a diff between
expected/privileges.out and results/privileges.out as an example of what I
mean:

ALTER SCHEMA regressns DROP DEFAULT PRIVILEGES ON TABLE ALL FROM
regressuser2;
***************
*** 895,903 ****
(1 row)

SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
! relname | relacl
! ----------+------------------------------------------------------
! acltest2 | {postgres=arwdDxt/postgres,regressgroup1=r/postgres}
(1 row)

CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;
--- 895,903 ----
(1 row)

SELECT relname, relacl FROM pg_class WHERE relname = 'acltest2';
! relname | relacl
! ----------+------------------------------------------
! acltest2 | {josh=arwdDxt/josh,regressgroup1=r/josh}
(1 row)

CREATE FUNCTION regressns.testfunc1() RETURNS int AS 'select 1;' LANGUAGE
sql;

Very minor stylistic or comment issues:

* There's a stray newline added in pg_class.h (no other changes were made to
that file by this patch)
* It feels to me like the comment "Continue with standard grant" in aclchk.c
interrupts the flow of the code, though such a comment was likely useful
when the patch was being written.
* pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class"
* The comment at the beginning of InsertPgClassTuple() in catalog/heap.c
should probably be updated to say that relation's ACLs aren't always NULL by
default
* copy_from in gram.y got changed to to_from, but to_from isn't ever used in
the default ACL grammar. I wondered if this was changed so you could use the
same TO/FROM code as COPY uses, and then you decided to hardcode TO and FROM?

In my perusal of the patch, I didn't see any code that screamed at me as
though it were a bad idea; quite likely there weren't any really bad ideas but
I can't say with confidence I'd have spotted them if there were. The addition
of both the NSPDEFACLOBJ_* defines alongside the NSP_ACL_OBJ_* defines kinda
made me think there were too many sets of constants that had to be kept in
sync, but I'm not sure that's much of an issue in reality, given how unlikely
it is that schema object types to which default ACLs should apply are likely
to be added or removed.

I don't know how patches that require catalog version changes are generally
handled; should the patch include that change?

More testing to follow.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2009-07-18 01:52:53 Re: [PATCH] DefaultACLs
Previous Message Andrew Dunstan 2009-07-18 01:09:20 Re: make check failure for 8.4.0