Re: Foreign table permissions and cloning

On Mon, Apr 25, 2011 at 2:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Apr 25, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not sure I quite understood what you were saying there, but I'm
>> coming around to the view that this is already 100% consistent with
>> the way views are handled:
>
>> rhaas=# create view v as select 1;
>> CREATE VIEW
>> rhaas=# grant delete on v to bob;
>> GRANT
>> rhaas=# grant delete on table v to bob;
>> GRANT
>
>> If that works for a view, it also ought to work for a foreign table,
>> which I think is what you were saying.
>
> Yeah, the existing precedent (not only for GRANT but for some other
> things like ALTER TABLE) is that a command that says "TABLE" is allowed
> to apply to other relation types if it makes sense to apply it.  It's
> only when you name some other object type that we get picky about the
> relkind matching exactly.  This is probably more historical than
> anything else, but it's the precedent and we shouldn't make foreign
> tables be the only thing not following the precedent.
>
>> So now I think this is just a documentation bug.
>
> If the code already works like that for foreign tables, then no
> behavioral change is needed.

OK, let's test that:

rhaas=# create foreign data wrapper dummy;
CREATE FOREIGN DATA WRAPPER
rhaas=# create server s1 foreign data wrapper dummy;
CREATE SERVER
rhaas=# create foreign table ft (a int) server s1;
CREATE FOREIGN TABLE
rhaas=# grant delete on ft to bob;
ERROR:  foreign table "ft" only supports SELECT privileges
rhaas=# grant delete on table ft to bob;
ERROR:  foreign table "ft" only supports SELECT privileges

So, nope, not the same.  Also for comparison:

rhaas=# create sequence blarg;
CREATE SEQUENCE
rhaas=# grant delete on blarg to bob;
WARNING:  sequence "blarg" only supports USAGE, SELECT, and UPDATE privileges
GRANT

This appears to be because ExecGrant_Relation() has this:
           else if (pg_class_tuple->relkind == RELKIND_FOREIGN_TABLE)           {               if (this_privileges &
~((AclMode)ACL_ALL_RIGHTS_FOREIGN_TABLE))               {                   ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),                           errmsg("foreign table \"%s\" only 
supports SELECT privileges",                                   NameStr(pg_class_tuple->relname))));               }
     } 

There's a similar stanza for sequences, but that one uses
ereport(WARNING...) rather than ereport(ERROR...).  We could either
remove that stanza entirely (making foreign tables consistent with
views) or change ERROR to WARNING (making it consistent with
sequences).

If we remove it entirely, then we'll presumably also want to remove
this chunk further down:
           else if (pg_class_tuple->relkind == RELKIND_FOREIGN_TABLE &&                    this_privileges &
~((AclMode)ACL_SELECT))           {               /* Foreign tables have the same restriction as sequences. */
    ereport(WARNING,                       (errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("foreigntable \"%s\" only supports 
SELECT column privileges",                               NameStr(pg_class_tuple->relname))));
this_privileges&= (AclMode) ACL_SELECT;           } 

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company