Re: [PATCH] DefaultACLs

Petr Jelinek <pjmodos@pjmodos.net> writes:
> [ latest version of DefaultACLs patch ]

I started looking through this patch, but found that it's not nearly
ready to commit :-(.  The big missing piece is that there's no pg_dump
support for default ACLs.  That's a bigger chunk of code than I have
time/interest to write, and I don't think I want to commit the feature
without it.  (I'm willing to commit without tab completion or any
psql \d command to show the defaults, but pg_dump just isn't optional.)

There is another large problem, too.  The patch seems to have
only half-baked support for global defaults (those not tied to a
specific schema) --- it looks like you can put them in, but half
of the code will ignore them or else fail while trying to use them.
This isn't just a matter of a few missed cases while coding, I think.
The generic issue that the code doesn't even think about addressing
is which default should apply when there's potentially more than one
applicable default?  As long as there's only global and per-schema
defaults, it's not too hard to decide that the latter take precedence
over the former; but I have no idea what we're going to do in order
to add any other features.  This seems like a sufficiently big
conceptual issue that it had better be resolved now, even if the first
version of the patch doesn't really need to deal with it.

Also, the GRANT DEFAULT PRIVILEGES thing just seems completely bizarre,
and I'm not convinced it has a sufficient use-case to justify such a
strange wart on GRANT.  I think we should drop it.  Or at least it needs
to be proposed and discussed as a separate feature.  Maybe it would seem
less strange if the syntax was "RESET PRIVILEGES ON object".

A couple of minor gripes:

Per recent discussion, names of system catalogs shouldn't be plural.

elog() is not suitable for user-facing errors.  For example in
ExecGrantStmt_Defaults, the grammar does not prohibit the unsupported
target object types, so you need to throw a nicer error there.  Or
else reject them in the grammar.  There seem to be a number of other
places where elog is used but the error is perfectly likely to be caused
by a user mistake.
        regards, tom lane