Re: [PATCH] DefaultACLs

Hi,

here's a (late, sorry about that) review:

== Trivia ==

Patch applies cleanly with a few 1 line offsets.

It's unified, not context, but that's trivial.

The patch adds some trailing whitespace, which is not good (git diff
shows it in red, it's easy to spot it). There's also one
hunk that's just an addition of a newline (in
src/backend/catalog/aclchk.c, -270,6 +291,7)

== Code ==

There's a few places where the following pattern is used:
if (!stmt->grantees)
whereas I think the project prefers:
stmt->grantees != NIL
Same for if (schemas) => if (schemas != NULL)

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.

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.

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.

Also, changing tests like if (!isNull) to if (relacl) makes it more
natural to
see sequences like if (relacl) { do-stuff; pfree(relacl); }

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:

alter default privileges for role test for role test grant select, insert on
table to test;

Maybe add a unit test for that.

A comment in dependency.c function getDefaultACLDescription with
something like
"shouldn't get here" in the default: switch branch could be useful,
cf. getRelationDescription.

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.

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.

The "unrecognized GrantStmt.objtype: %d" error message needs better
wording I
think.

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

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

== Testing ==

Tab completion is not up to speed - annoying ;)

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.

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.

== Docs ==

Need to document that FOR USER also works (as a synonym for FOR ROLE) in the
synopsis of ALTER DEFAULT PRIVILEGES.

Add examples of usage of the FOR USER/FOR ROLE syntax and explain what
they do.

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;

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.

== Other ==

I'm not really sure what's the use of GRANT DEFAULT PRIVILEGES... Is it for
backward-sanitizing existing databases?

I haven't tested performance at all, I thinks it doesn't make sense for
a patch
like this.

Don't know about the relationship with the spec, I suspect there's no such
thing in there. But the feature has been reworked to match what the
community
wanted and I think that the syntax is quite good and the use cases are
there.

Not tested on Windows, probably not important.

Having said all that, I'm moving the patch to "Waiting on author".

Best,
Jan