Jan Urbański napsal(a):
Hi,here's a (late, sorry about that) review:
Thanks for the comprehensive review!
It's not, I have git configured to produce context diffs (see http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git ).It's unified, not context, but that's trivial.
== Code == There's a few places where the following pattern is used: if (!stmt->grantees) whereas I think the project prefers: stmt->grantees != NIL
Ok changed that.
I'm not sure if this pattern in src/backend/catalog/aclchk.c is the best
option:
if (rolenames == NIL)
rolenames = lappend(rolenames, makeString(pstrdup("")));
if (nspnames == NIL)
nspnames = lappend(nspnames, makeString(pstrdup("")));
Appending an empty string and then checking in strlen of the option
value is 0
is ugly.
I changed that, I had to change the way SetDefaultACLs is called a bit anyway (to fix the problem with dependencies you mentioned below).
In SetDefaultACLs the OidIsValid(roleId) is not necessary, maybe better put in assert(oidIsValid). The caller always puts a valid rolename in there. Or maybe even better: make the caller pass an InvalidOid for the role if no is specified. This way the handling arguments is more uniform between SetDefaultACLs and ExecDefaultACLsStmt.
Same as above.
I agree, btw it actually does not save anything in InsertPgClassTuple, but it does save few lines in ProcedureCreate.The logic in get_default_acl and pg_namespace_object_default_acl could be improved, for instance get_default_acl checks if the result of the scan is null and if is, returns NULL. At the same time, the only calling function, pg_namespace_object_default_acl checks the isNull flag instead of just checking if the result is NULL. Also, pg_namespace_object_default_acl could just do without the isNull out parameter and the same goes for get_default_acl. Just return NULL to indicate an invalid result and declare a isNull in get_default_acl locally to use it in heap_getattr. This also saves some lines in InsertPgClassTuple.
Also, in InsertPgClassTuple change the order of the branches: + if (isNull) + nulls[Anum_pg_class_relacl - 1] = true; + else + values[Anum_pg_class_relacl - 1] = PointerGetDatum(relacl); to have the same pattern as the next if statement.
Done.
In ExecDefaultACLsStmt this fragment:
else if (strcmp(defel->defname, "roles") == 0)
{
if (rolenames)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options")));
drolenames = defel;
}
Should test if (drolenames), because currently it's possible to do:
Typo, fixed.
A comment in dependency.c function getDefaultACLDescription with something like "shouldn't get here" in the default: switch branch could be useful, cf. getRelationDescription.
Ok.
In ExecGrantDefaults_Relation there's a hunk: + if (isNull) + elog(ERROR, "no DEFAULT PRIVILEGES for relation"); Maybe you could move it higher, no need to do other stuff if it's going to fail afterwards anyway.
Ok, I moved it just after owner check.
They don't really, they operate on different tables and the only partly shared part is the indexes and acl dependency update.ExecGrantDefaults_Function and ExecGrantDefaults_Relation could maybe share code? They look quite similar, although it might not be so easy to factor out common functionality.
Well, that exact same message is in other two places in original code, I just copy pasted it.The "unrecognized GrantStmt.objtype: %d" error message needs better wording I think.
Now that I fixed DefaultACLs removal on role drop this is no longer true (previously it was only dropped with schema a leftover from original design). Also revoking everything is not same as having no DefaultACLs, with no DefaultACLs current behavior is used (owner gets all privs and public might get something depending on object type).No code patch removes rows from pg_default_acls, so it might accumulate cruft. Maybe a drop default privileges? Or maybe revoking all would delete the row instead of setting it? It has the same meaning, I guess...
== Compiling and installing == My gcc complains about gram.y: In function ‘base_yyparse’: gram.y:1128: warning: assignment from incompatible pointer type gram.y:1135: warning: passing argument 1 of ‘lappend’ from incompatible pointer type gram.y:1135: warning: assignment from incompatible pointer type gram.y:1136: warning: assignment from incompatible pointer type
Fixed.
Regression tests fail because of the username mismatch ! DETAIL: drop cascades to default acls for role postgres on new relation in namespace regressns --- 951,957 ---- ! DETAIL: drop cascades to default acls for role wulczer on new relation in namespace regressns
Removed that part from regression.
== Testing ==
The functionality worked as advertised, although I was able to do the
following:
postgres=# create role test login;
CREATE ROLE
postgres=# \c - test
psql (8.5devel)
You are now connected to database "postgres" as user "test".
postgres=> alter default privileges grant select on table to test;
ALTER DEFAULT PRIVILEGES
postgres=> \c - wulczer
psql (8.5devel)
You are now connected to database "postgres" as user "wulczer".
postgres=# drop role test;
DROP ROLE
postgres=# select * from pg_default_acls ;
defaclrole | defaclnamespace | defaclobjtype | defacllist
------------+-----------------+---------------+-----------------------
16384 | 0 | r | {16384=arwdDxt/16384}
The numeric defaclrole and defacllist don't look good.
Fixed, DefaultACLs for role are now dropped in DropRole.
While testing I also got a "unexpected object type: 1248" from src/backend/catalog/pg_shdepend.c, but was unable to reproduce it. This happened after I did a combination of DROP OWNED BY and REASSIGN OWNED for a role that has membership in other roles and that all had default privileges in a schema.
Yeah that was because I was adding ACL dependencies and wasn't removing them in DROP OWNED BY. Fixed. This is why I had to change the way SetDefaultACLs is called, so it could be called from DROP OWNED BY.
== Docs == Need to document that FOR USER also works (as a synonym for FOR ROLE) in the synopsis of ALTER DEFAULT PRIVILEGES.
Added.
Add examples of usage of the FOR USER/FOR ROLE syntax and explain what they do.
Added simple example.
In the ALTER DEFAULT PRIVILEGES synopsis there's a copy-paste-o because it says you can do: ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DEFAULT PRIVILEGES ON TABLE TO test;
Eh, right, fixed.
The wording of "with grant options parameter is not applicable" in the sql-grant page should mentiont that you also can't do: GRANT DEFAULT PRIVILEGES ON s.test2 TO test; so it should be indicated the "TO rolename" is also not applicable.
Done.
== Other == I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for backward-sanitizing existing databases?
Yes (especially in combination with GRANT ON ALL if it gets in).
Not tested on Windows, probably not important.
I did test it on Windows ;)
I'll changed it back to "needs review" since I made quite a few changes (per your review).Having said all that, I'm moving the patch to "Waiting on author".
-- Regards Petr Jelinek (PJMODOS)
defacl-2009-09-22.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
