Received: from localhost (unknown [200.46.208.211])
	by mail.postgresql.org (Postfix) with ESMTP id ADBE563326F
	for <pgsql-hackers-postgresql.org@mail.postgresql.org>;
	Sun, 20 Sep 2009 11:50:12 -0300 (ADT)
Received: from mail.postgresql.org ([200.46.204.86])
	by localhost (mx1.hub.org [200.46.208.211]) (amavisd-maia, port 10024)
	with ESMTP id 99988-03
	for <pgsql-hackers-postgresql.org@mail.postgresql.org>;
	Sun, 20 Sep 2009 14:49:52 +0000 (UTC)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from fugue.toroid.org (fugue.toroid.org [85.10.196.113])
	by mail.postgresql.org (Postfix) with ESMTP id D8B906330AD
	for <pgsql-hackers@postgresql.org>;
	Sun, 20 Sep 2009 11:50:01 -0300 (ADT)
Received: from penne.toroid.org (penne-vpn [10.8.0.6])
	by fugue.toroid.org (Postfix) with ESMTP id 46D0F55838E;
	Sun, 20 Sep 2009 16:49:58 +0200 (CEST)
Received: by penne.toroid.org (Postfix, from userid 1000)
	id B06013880DB; Sun, 20 Sep 2009 20:20:11 +0530 (IST)
Date: Sun, 20 Sep 2009 20:20:11 +0530
From: Abhijit Menon-Sen <ams@toroid.org>
To: pgsql-hackers@postgresql.org
Cc: Petr Jelinek <pjmodos@pjmodos.net>
Subject: Re: GRANT ON ALL IN schema
Message-ID: <20090920145011.GA24273@toroid.org>
References: <4A37BF63.50008@pjmodos.net> <4A37E122.8070303@pjmodos.net>
	<4A38A956.8080600@pjmodos.net> <4A4DE104.8090605@pjmodos.net>
	<a301bfd90907170254re7dd52es9fedb007376d8055@mail.gmail.com>
	<4A6059B4.5010004@pjmodos.net> <4A607997.3030305@pjmodos.net>
	<21542.1249492707@sss.pgh.pa.us> <4A7F56A0.5060705@pjmodos.net>
	<4A7F5853.5010506@pjmodos.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4A7F5853.5010506@pjmodos.net>
X-Virus-Scanned: Maia Mailguard 1.0.1
X-Spam-Status: No, hits=-2.599 tagged_above=-10 required=5
	tests=BAYES_00=-2.599
X-Spam-Level: 
X-Archive-Number: 200909/1339
X-Sequence-Number: 146145

(This is a partial review of the grantonall-20090810v2.diff patch posted
by Petr Jelinek on 2009-08-10 (hi PJMODOS!). See
http://archives.postgresql.org/message-id/4A7F5853.5010506@pjmodos.net
for the original message.)

I have not yet been able to do a complete review of this patch, but I am
posting this because I'll be travelling for a week starting tomorrow. My
comments are based mostly on reading the patch, and not on any intensive
testing of the feature. I have left the patch status unchanged at "needs
review", although I think it's close to "ready for committer".

I really like this patch. It's easy to understand and written in a very
straightforward way, and addresses a real need that comes up time and
again on various support fora. I have only a couple of minor comments.

1. The patch did apply to HEAD and build cleanly, but there are now a
   couple of minor (documentation) conflicts. (Sorry, I would have fixed
   them and reposted a patch, but I'm running out of time right now.)

> *** a/doc/src/sgml/ref/grant.sgml
> --- b/doc/src/sgml/ref/grant.sgml
> [...]
> 
>     <para>
> +    There is also the possibility of granting permissions to all objects of
> +    given type inside one or multiple schemas. This functionality is supported
> +    for tables, views, sequences and functions and can done by using
> +    ALL {TABLES|SEQUENCES|FUNCTIONS} IN SCHEMA schemaname syntax in place
> +    of object name.
> +   </para>
> + 
> +   <para>

2. Here I suggest the following wording:

    <para>
    You can also grant permissions on all tables, sequences, or
    functions that currently exist within a given schema by specifying
    "ALL {TABLES|SEQUENCES|FUNCTIONS} IN SCHEMA schemaname" in place of
    an object name.
    </para>

3. I believe MySQL's "grant all privileges on foo.* to someone" grants
   privileges on all existing objects in foo _but also_ on any objects
   that may be created later. This patch only gives you a way to grant
   privileges only on the objects currently within a schema. I strongly
   prefer this behaviour myself, but I do think the documentation needs
   a brief mention of this fact, to avoid surprising people. That's why
   I added "that currently exist" to (2), above. Maybe another sentence
   that specifically says that objects created later are unaffected is
   in order. I'm not sure.

-- ams