Joshua Tolley wrote:
First, as you've already pointed out, this needs documentation.
/me looks at Stephen ;)
As we discussed outside of list, my compiler didn't picked that one because I didn't use --enable-cassert .Once I added the missing semicolon mentioned earlier, it applies and builds
Oh I thought they are always run under *database user* postgres, my bad, I reworked them and the are probably better as a result.fine. The regression tests, however, seem to assume that they'll be run as the postgres user, and the privileges test failed.
Fixed, probably I either pressed enter by mistake while viewing that file or it was some merging problem when updating my working copy.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.
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.
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.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.
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)
defaultacls.diff.gz
Description: Unix tar archive
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
