Re: [PATCH] DefaultACLs - Mailing list pgsql-hackers

Joshua Tolley wrote:
> First, as you've already pointed out, this needs documentation.
>
/me looks at Stephen ;)

> Once I added the missing semicolon mentioned earlier, it applies and builds
>
As we discussed outside of list, my compiler didn't picked that one
because I didn't use --enable-cassert .

> fine. The regression tests, however, seem to assume that they'll be run as the
> postgres user, and the privileges test failed.
Oh I thought they are always run under *database user* postgres, my bad,
I reworked them and the are probably better as a result.

> 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)
>
Fixed, probably I either pressed enter by mistake while viewing that
file or it was some merging problem when updating my working copy.

> * 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.
>
Ok I removed that comment.

> * pg_namespace_default_acl.h:71 should read "objects stored *in* pg_class"
>
Fixed.

> * 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
>
Done.

> * 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?
>
Yes, that's more or less what happened.

> 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.
>
Well the problem you see is not really a problem IMHO because most of
code I've seen does not use actual catalog values inside gram.y/parser
so I didn't use them either.

But there is one problem there that also affects GRANT ON ALL patch and
that was discussed previously (see
http://archives.postgresql.org/pgsql-hackers/2009-07/msg00957.php and
the rest of the thread from there). One (or both) of those patches will
have to be adjusted but only commiter can decide that IMHO (I am happy
to make any necessary changes but I really don't know which of the 3
solutions is better).

> I don't know how patches that require catalog version changes are generally
> handled; should the patch include that change?
>
Than can reasonably be done only at commit time so it's up to commiter.

I attached updated version of the patch per your comments. Let's hope I
didn't introduce new problems :)

Thanks for your time.

--
Regards
Petr Jelinek (PJMODOS)