Re: [PATCH] DefaultACLs - Mailing list 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()
RETURNSint 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
'select1;' 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
sucha 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'sACLs 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
thiswas 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