Re: [PATCH] DefaultACLs

On Fri, Jul 17, 2009 at 9:46 PM, Joshua Tolley<eggyknap(at)gmail(dot)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