GRANT ON ALL IN schema

Petr Jelinek wrote:

Anyway back to my thoughts about this patch. First of all I see problem
with the proposed syntax. For this syntax I think TABLES (FUNCTIONS,
SEQUENCES, etc) would have to be added to keywords which is problematic
because there are views named tables, sequences, views in
information_schema so we can't really make them keywords. I have no idea
how to get around this and I don't see good alternative syntax either.
This is main and only real problem I have.

Erm, seems like the problem was just me overlooking something in gram.y
(I forgot to add those keywords to unreserved_keyword) so no real
problems, but I'd still like to hear answers to those other questions in
my previous email.

--
Regards
Petr Jelinek (PJMODOS)

So, here is the first version of the patch.
It includes functionality itself, simple regression test and also very
simple documentation.

The patch allows "GRANT ON ALL TABLES/VIEWS/FUNCTIONS/SEQUENCES IN
schemaname, schemaname2 TO username" and same thing for REVOKE.
Words TABLES, VIEWS, FUNCTIONS and SEQUENCES were added as unreserved
keywords. Unfortunately I was unable to create syntax with optional
SCHEMA keyword after IN (shift/reduce conflicts), if it's needed maybe
somebody with better bison knowledge might add it.
Also since this patch introduces VIEWS as object with grantable
privileges, I added GRANT ON VIEW foo syntax which is more or less
synonymous to GRANT ON TABLE foo syntax. It felt weird to have GRANT ON
ALL VIEWS but not GRANT ON VIEW.

Any comments/suggestions are welcome (I especially wonder if the use of
list_union_ptr is acceptable).

--
Regards
Petr Jelinek (PJMODOS)

On Wednesday 17 June 2009 11:29:10 Petr Jelinek wrote:

The patch allows "GRANT ON ALL TABLES/VIEWS/FUNCTIONS/SEQUENCES IN
schemaname, schemaname2 TO username" and same thing for REVOKE.
Words TABLES, VIEWS, FUNCTIONS and SEQUENCES were added as unreserved
keywords. Unfortunately I was unable to create syntax with optional
SCHEMA keyword after IN (shift/reduce conflicts), if it's needed maybe
somebody with better bison knowledge might add it.

I think you should design this with a bit wider scope. Instead of just "all
tables in this schema", think "all tables satisfying some condition". It has
been requested, for example, to be able to grant on all tables that match a
pattern.

Also since this patch introduces VIEWS as object with grantable
privileges, I added GRANT ON VIEW foo syntax which is more or less
synonymous to GRANT ON TABLE foo syntax. It felt weird to have GRANT ON
ALL VIEWS but not GRANT ON VIEW.

As far as GRANT is concerned, a view is a table, so I would omit the
VIEW/VIEWS stuff completely.

Peter,

* Peter Eisentraut (peter_e@gmx.net) wrote:

Also since this patch introduces VIEWS as object with grantable
privileges, I added GRANT ON VIEW foo syntax which is more or less
synonymous to GRANT ON TABLE foo syntax. It felt weird to have GRANT ON
ALL VIEWS but not GRANT ON VIEW.

As far as GRANT is concerned, a view is a table, so I would omit the
VIEW/VIEWS stuff completely.

I would disagree with this. While an explicit GRANT doesn't need to
care, because you can't have a view and a table with the same name, I
feel *users* (like me) make a distinction there and may want to limit
the grant to just views or just tables.

What we do here will also impact the DefaultACL system that I'm working
on since I think we should be consistant between these two systems.

http://wiki.postgresql.org/wiki/DefaultACL

I don't like the idea that a 'GRANT ALL' would actually change default
ACLs for a schema though. These are two separate and distinct things-
one is implementing a change to existing objects, the other is setting a
default for new objects. Mixing them would lead to confusion.

Thanks,

Stephen

Peter Eisentraut <peter_e@gmx.net> writes:

I think you should design this with a bit wider scope. Instead of just "all
tables in this schema", think "all tables satisfying some condition". It has
been requested, for example, to be able to grant on all tables that match a
pattern.

I'm against that. Functionality of that sort is available now if you
really need it (write a plpgsql loop around an EXECUTE) and it's fairly
hard to see a clean syntax that is significantly more general than
"GRANT ON schema.*". In particular I strongly advise against getting
into supporting user-defined predicates in GRANT. There are good
reasons for not having utility statements evaluate random expressions.

regards, tom lane

Peter Eisentraut wrote:

I think you should design this with a bit wider scope. Instead of just "all
tables in this schema", think "all tables satisfying some condition". It has
been requested, for example, to be able to grant on all tables that match a
pattern.

Well, that's certainly possible to do. But I am not sure what kind of
conditions (besides the name), nor I don't see any sane grammar for this
(maybe something like GRANT SELECT ON TABLE WHERE NAME LIKE '%foo' but
that's far to weird). That all tables in this schema thing was agreed on
on this mailing list and put on TODO so I thought it's something people
want in this form (I know I needed it myself).

As far as GRANT is concerned, a view is a table, so I would omit the
VIEW/VIEWS stuff completely.

This maybe true for underlying implementation and for granting
permissions to a single object, but you might want to grant select on
all views without granting it to all tables in the schema. And as I said
having VIEWS and not VIEW just seems weird. Also I don't see why you
would want to add possibility of specifying stricter conditions for
objects and at the same time remove possibility of distinguishing
between tables and views.

--
Regards
Petr Jelinek (PJMODOS)

Petr,

* Petr Jelinek (pjmodos@pjmodos.net) wrote:

So, here is the first version of the patch.

Neat, thanks! Some initial comments:

You should read through this:
http://wiki.postgresql.org/wiki/Submitting_a_Patch

First big thing is that the patch should be a context diff. I would
also recommend you put it up on the CommitFest wiki if it's not there
yet. You might also write up a wiki page on it and link to it from the
8.5 WIP section under
http://wiki.postgresql.org/wiki/Developer_and_Contributor_Resources

The http://wiki.postgresql.org/wiki/Developer_FAQ can also help if you
havn't checked it out yet.

It includes functionality itself, simple regression test and also very
simple documentation.

Excellent!

Any comments/suggestions are welcome (I especially wonder if the use of
list_union_ptr is acceptable).

I'll try to take a look at the actual patch in more detail later this
week.

Thanks!

Stephen

Stephen Frost wrote:

I don't like the idea that a 'GRANT ALL' would actually change default
ACLs for a schema though. These are two separate and distinct things-
one is implementing a change to existing objects, the other is setting a
default for new objects. Mixing them would lead to confusion.

It doesn't. I stated that I am not implementing the second part of the
TODO item in my first email specifically for this reason (the second
part was GRANT ON NEW TABLES).

--
Regards
Petr Jelinek (PJMODOS)

Stephen Frost wrote:

http://wiki.postgresql.org/wiki/Submitting_a_Patch

First big thing is that the patch should be a context diff. I would

It is context diff, at least I think so, I followed the instructions on
wiki on how to make context patch from git repo.

also recommend you put it up on the CommitFest wiki if it's not there
yet. You might also write up a wiki page on it and link to it from the
8.5 WIP section under
http://wiki.postgresql.org/wiki/Developer_and_Contributor_Resources

Will do.

I'll try to take a look at the actual patch in more detail later this
week.

Thanks.

--
Regards
Petr Jelinek (PJMODOS)

* Petr Jelinek (pjmodos@pjmodos.net) wrote:

Stephen Frost wrote:

I don't like the idea that a 'GRANT ALL' would actually change default
ACLs for a schema though. These are two separate and distinct things-
one is implementing a change to existing objects, the other is setting a
default for new objects. Mixing them would lead to confusion.

It doesn't. I stated that I am not implementing the second part of the
TODO item in my first email specifically for this reason (the second
part was GRANT ON NEW TABLES).

Right.. I was arguing against a few folks who had mentioned they'd like
to see that on IRC and in the past, not against your implementation.

Thanks,

Stephen

On Wednesday 17 June 2009 17:15:04 Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I think you should design this with a bit wider scope. Instead of just
"all tables in this schema", think "all tables satisfying some
condition". It has been requested, for example, to be able to grant on
all tables that match a pattern.

I'm against that. Functionality of that sort is available now if you
really need it (write a plpgsql loop around an EXECUTE) and it's fairly
hard to see a clean syntax that is significantly more general than
"GRANT ON schema.*". In particular I strongly advise against getting
into supporting user-defined predicates in GRANT. There are good
reasons for not having utility statements evaluate random expressions.

Why don't we tell people to write a plpgsql loop for the schema.* case as
well?

I haven't seen any evidence that the schema.* case is more common than other
bulk DDL cases like "matches pattern" or "owned by $user" or "grant on all
functions that are not security definer" etc.

Peter Eisentraut <peter_e@gmx.net> writes:

Why don't we tell people to write a plpgsql loop for the schema.* case as
well?

Indeed, why not? This all seems much more like gilding the lily than
delivering useful new capability. The default-ACL stuff that Stephen
is working on seems far more important in practice.

regards, tom lane

* Petr Jelinek (pjmodos@pjmodos.net) wrote:

It is context diff, at least I think so, I followed the instructions on
wiki on how to make context patch from git repo.

err, sorry, tbh I just looked at the 'diff --git' line and didn't see
any '-c'.. Trying to do too much at one time, I'm afraid. :)

Thanks,

Stephen

Tom Lane erote:

Peter Eisentraut <peter_e@gmx.net> writes:

Why don't we tell people to write a plpgsql loop for the schema.* case as
well?

Indeed, why not? This all seems much more like gilding the lily than
delivering useful new capability. The default-ACL stuff that Stephen
is working on seems far more important in practice.

I agree that Default ACLs are more important and I already offered
Stephen help on that. But I've seen countless requests for granting on
all tables to a user and I already got some positive feedback outside of
the list, so I believe there is demand for this. Also to paraphrase you
Tom, by that logic you can tell people to write half of administration
functionality as plpgsql functions.

--
Regards
Petr Jelinek (PJMODOS)

2009/6/17 Petr Jelinek <pjmodos@pjmodos.net>:

I agree that Default ACLs are more important and I already offered Stephen
help on that. But I've seen countless requests for granting on all tables to
a user and I already got some positive feedback outside of the list, so I
believe there is demand for this. Also to paraphrase you Tom, by that logic
you can tell people to write half of administration functionality as plpgsql
functions.

Indeed.

How to do default ACLs and wildcards for GRANT is by far the most
common question asked by our customers. And they don't understand why
it's not by default in PostgreSQL.

Installing a script/function for that on every database is just painful.

--
Guillaume

Isn't the answer to grant permissions to a role and then just put
people in that role?

--
Greg

On 17 Jun 2009, at 17:25, Guillaume Smet <guillaume.smet@gmail.com>
wrote:

Greg Stark wrote:

Isn't the answer to grant permissions to a role and then just put
people in that role?

Still have to give permissions at least to that role.

--
Regards
Petr Jelinek (PJMODOS)

On Wed, Jun 17, 2009 at 12:25 PM, Guillaume
Smet<guillaume.smet@gmail.com> wrote:

2009/6/17 Petr Jelinek <pjmodos@pjmodos.net>:

I agree that Default ACLs are more important and I already offered Stephen
help on that. But I've seen countless requests for granting on all tables to
a user and I already got some positive feedback outside of the list, so I
believe there is demand for this. Also to paraphrase you Tom, by that logic
you can tell people to write half of administration functionality as plpgsql
functions.

Indeed.

How to do default ACLs and wildcards for GRANT is by far the most
common question asked by our customers. And they don't understand why
it's not by default in PostgreSQL.

Installing a script/function for that on every database is just painful.

It's not just GRANT, either. I have a script that synchronizes data
from <other database product> into PostgreSQL. It runs out of cron.
I actually had to set it up so that it counts the total number of rows
that it has inserted and fires of an ANALYZE when it hits a certain
threshold (that might not be necessary with autovacuum, but this is
8.1); otherwise, the statistics can get so far from reality that the
sync script never finishes, because the later stages of the sync query
local data modified by earlier stages of the sync. This is not a
joke; when there are heavy data modifications, the script MUST fire an
ANALYZE midway through to complete in a reasonable amount of time.

Now it just so happens that this application runs inside its own
schema, and that it doesn't have permission to vacuum any of the other
schemas, including the catalog tables. So what do you think happens
when it kicks off an ANALYZE? A huge pile of warning messages.

Now, since I've been reading pgsql-hackers religiously for a year now,
I know that it's very easy to solve this problem by writing a table to
issue a query against pg_class and then use quote_ident() to build up
a query that we can EXECUTE from within a pl/pgsql loop. However, I
certainly didn't know how to do that when I wrote the script two and a
half years ago, at which time I had only about six years of experience
with the product. Before I started reading -hackers, I relied on
reading the fine manual:

http://www.postgresql.org/docs/8.3/static/sql-analyze.html

...which doesn't describe how to do this. So I didn't know. But if
the file manual had included the syntax "ANALYZE SCHEMA blat", I
certainly would have used it, and thus avoided getting 10 emails a
week from my cron job for the past two-and-half years.

What to do about wildcards is a stickier wicket, and maybe we need to
decide that first, but I really don't think we should be discouraging
anyone from investigating this stuff and trying to come up with good
solutions. There will always be some people for whom a custom
PL/pgsql function that directly accesses the catalog tables is the
only workable answer, but we can make PostgreSQL a whole lot easier to
use by reducing the need to do that for simple cases.

...Robert

On Wednesday 17 June 2009 20:27:20 Robert Haas wrote:

What to do about wildcards is a stickier wicket, and maybe we need to
decide that first, but I really don't think we should be discouraging
anyone from investigating this stuff and trying to come up with good
solutions. There will always be some people for whom a custom
PL/pgsql function that directly accesses the catalog tables is the
only workable answer, but we can make PostgreSQL a whole lot easier to
use by reducing the need to do that for simple cases.

I'm all for investigating it. I just have my doubts that "grant on all tables
in schema X" is a sufficiently general use case, even if you only concentrate
on the simple cases.