On Fri, Jul 17, 2009 at 9:46 PM, Joshua Tolley<eggyk...@gmail.com> wrote: > 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.
So are these warts fixed in the latest revision of this patch? http://archives.postgresql.org/pgsql-hackers/2009-07/msg01216.php I am gathering that this patch is still a bit of a WIP. I think it might be best to mark it returned with feedback and let Petr resubmit for the next CommitFest when it is closer to being done. But I don't want to do that if it's really already to go now. I am also a bit unsure as to whether Josh Tolley is still conducting a more in-depth review. Josh? Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
