Re: Foreign table permissions and cloning

Hi,

I've noticed some weirdness when trying to grant various types of
permissions on a foreign table and thought I'd report it here:

postgres=# \d stuff
Foreign table "public.stuff"
Column | Type | Modifiers
--------+---------+-----------
id | integer |
colour | text |
animal | text |
Server: file

postgres=# GRANT SELECT (colour) ON FOREIGN TABLE stuff TO user_a;
ERROR: column privileges are only valid for relations
postgres=# GRANT SELECT (colour) ON TABLE stuff TO user_a;
GRANT
postgres=# GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_a;
ERROR: syntax error at or near "FOREIGN"
LINE 1: GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_...
^
Granting select for all tables in a schema to a user will affect
foreign tables however. And column-level permissions work with
foreign tables if you refer to them as regular tables in the
GRANT/REVOKE statement.

Using the term FOREIGN TABLE in a GRANT statement isn't documented.
I suspect this will need its own entry in the syntax definition
section of the GRANT and REVOKE reference pages.

I also noticed this doesn't work:

postgres=# CREATE TABLE animals (LIKE stuff);
ERROR: inherited relation "stuff" is not a table

Since LIKE doesn't maintain any sort of link with the table like
INHERITS does, it would be nice if this could work in future.

Thanks

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1 April 2011 00:54, Thom Brown <thom(at)linux(dot)com> wrote:
> Hi,
>
> I've noticed some weirdness when trying to grant various types of
> permissions on a foreign table and thought I'd report it here:
>
> postgres=# \d stuff
>  Foreign table "public.stuff"
>  Column |  Type   | Modifiers
> --------+---------+-----------
>  id     | integer |
>  colour | text    |
>  animal | text    |
> Server: file
>
> postgres=# GRANT SELECT (colour) ON FOREIGN TABLE stuff TO user_a;
> ERROR:  column privileges are only valid for relations
> postgres=# GRANT SELECT (colour) ON TABLE stuff TO user_a;
> GRANT
> postgres=# GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_a;
> ERROR:  syntax error at or near "FOREIGN"
> LINE 1: GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_...
>                            ^
> Granting select for all tables in a schema to a user will affect
> foreign tables however.  And column-level permissions work with
> foreign tables if you refer to them as regular tables in the
> GRANT/REVOKE statement.
>
> Using the term FOREIGN TABLE in a GRANT statement isn't documented.
> I suspect this will need its own entry in the syntax definition
> section of the GRANT and REVOKE reference pages.
>
> I also noticed this doesn't work:
>
> postgres=# CREATE TABLE animals (LIKE stuff);
> ERROR:  inherited relation "stuff" is not a table
>
> Since LIKE doesn't maintain any sort of link with the table like
> INHERITS does, it would be nice if this could work in future.

Also, there probably needs to be some elaboration of how a NOT NULL
declaration operates on a foreign table column on the CREATE FOREIGN
TABLE reference page. From what I can see, if the foreign table
cannot be modified such as those defined using the file_fdw handler,
it bears no relevance, and if the foreign table can be written to, it
won't prevent NULL values being returned if they're already in there,
just prevent them being entered (presumably). It also won't validate
data in the writable foreign table upon its creation.

And another problem I've found is if you try to create a table named
the same as a foreign table, and you use the IF NOT EXISTS clause:

postgres=# CREATE TABLE IF NOT EXISTS animals (id serial, stuff text);
NOTICE: CREATE TABLE will create implicit sequence "animals_id_seq1"
for serial column "animals.id"
NOTICE: relation "animals" already exists, skipping
CREATE TABLE
postgres=# CREATE TABLE IF NOT EXISTS stuff (id serial, stuff text);
NOTICE: CREATE TABLE will create implicit sequence "stuff_id_seq" for
serial column "stuff.id"
NOTICE: relation "stuff" already exists, skipping
ERROR: referenced relation "stuff" is not a table

The reverse doesn't error though, where you attempt to create a
foreign table named the same as a regular table using IF NOT EXISTS.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, 1 Apr 2011 00:54:18 +0100
Thom Brown <thom(at)linux(dot)com> wrote:
> I've noticed some weirdness when trying to grant various types of
> permissions on a foreign table and thought I'd report it here:
>
> postgres=# \d stuff
> Foreign table "public.stuff"
> Column | Type | Modifiers
> --------+---------+-----------
> id | integer |
> colour | text |
> animal | text |
> Server: file
>
> postgres=# GRANT SELECT (colour) ON FOREIGN TABLE stuff TO user_a;
> ERROR: column privileges are only valid for relations
> postgres=# GRANT SELECT (colour) ON TABLE stuff TO user_a;
> GRANT
> postgres=# GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_a;
> ERROR: syntax error at or near "FOREIGN"
> LINE 1: GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA public TO user_...
> ^
> Granting select for all tables in a schema to a user will affect
> foreign tables however. And column-level permissions work with
> foreign tables if you refer to them as regular tables in the
> GRANT/REVOKE statement.
>
> Using the term FOREIGN TABLE in a GRANT statement isn't documented.
> I suspect this will need its own entry in the syntax definition
> section of the GRANT and REVOKE reference pages.

In addition to the 2nd GRANT above, "GRANT SELECT (colour) ON stuff TO
user_a" (omitting TABLE) will succeed too because parser assumes that
the target object is a regular table if object type was TABLE or
omitted. This inconsistent behavior would be an oversight and need to
be fixed.

How about to drop "GRANT xxx ON FOREIGN TABLE foo" syntax support and
use "GRANT xxx ON [TABLE] foo" for foreign tables? ISTM that "ON
FOREIGN TABLE" specification is useless because possible privilege
type would be same as TABLE.

In this approach, "FOREIGN TABLE" (object type) is removed from
privilege_target of gram.y. With this change, parser can't determine
actual object type (ACL_OBJECT_RELATION or ACL_OBJECT_FOREIGN_TABLE),
but it wouldn't be problem because object type will be retrieved in
ExecGrant_Relation() for validate privilege type.

Probabry we should mention in GRANT documents that ALL TABLES
IN SCHEMA is considered to include foreign tables.

Attached patch includes removing GRANT ON FOREIGN TABLE syntax fix,
tab-completion fix, GRANT documents fix and additional regression
tests.

Regards,
--
Shigeru Hanada

On Fri, 1 Apr 2011 01:24:20 +0100
Thom Brown <thom(at)linux(dot)com> wrote:
> Also, there probably needs to be some elaboration of how a NOT NULL
> declaration operates on a foreign table column on the CREATE FOREIGN
> TABLE reference page. From what I can see, if the foreign table
> cannot be modified such as those defined using the file_fdw handler,
> it bears no relevance, and if the foreign table can be written to, it
> won't prevent NULL values being returned if they're already in there,
> just prevent them being entered (presumably). It also won't validate
> data in the writable foreign table upon its creation.

NOT NULL constraint on foreign table is just declaration and can't
force data integrity. And I noticed that CREATE FOREIGN TABLE
document doesn't mention that serial and bigserial can't be used in
foreign table. Please see foreign_table_doc.patch for this fix.

For constraint on foreign tables, once query-time-constraint was
considered, but such overhead would not be ignorable.

> And another problem I've found is if you try to create a table named
> the same as a foreign table, and you use the IF NOT EXISTS clause:
>
> postgres=# CREATE TABLE IF NOT EXISTS animals (id serial, stuff text);
> NOTICE: CREATE TABLE will create implicit sequence "animals_id_seq1"
> for serial column "animals.id"
> NOTICE: relation "animals" already exists, skipping
> CREATE TABLE
> postgres=# CREATE TABLE IF NOT EXISTS stuff (id serial, stuff text);
> NOTICE: CREATE TABLE will create implicit sequence "stuff_id_seq" for
> serial column "stuff.id"
> NOTICE: relation "stuff" already exists, skipping
> ERROR: referenced relation "stuff" is not a table
>
> The reverse doesn't error though, where you attempt to create a
> foreign table named the same as a regular table using IF NOT EXISTS.

Using int instead of serial or omitting "if not exists" prevends the
error, so I researched root cause.

CREATE TABLE with serial column is transformed into 3 DDLs:

(1) CREATE SEQUENCE, for serial column
(2) CREATE TABLE, skipped if table exists with same name
(3) ALTER SEQUENCE OWNED BY, associate sequence with table

This error occurs in (3) because process_owned_by() misunderstand
that existing table is new owner, but it's a foreign server and
shouldn't be used as owner. So same error occurs if the existing
relation was an index or a sequence.

Regards,
--
Shigeru Hanada

On 1 April 2011 12:57, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> NOT NULL constraint on foreign table is just declaration and can't
> force data integrity.  And I noticed that CREATE FOREIGN TABLE
> document doesn't mention that serial and bigserial can't be used in
> foreign table.  Please see foreign_table_doc.patch for this fix.

I'd be inclined to generalise it to say that default values can't be
used on a foreign table, and then say that as a result, serial and
bigserial can't be used.

> Using int instead of serial or omitting "if not exists" prevends the
> error, so I researched root cause.
>
> CREATE TABLE with serial column is transformed into 3 DDLs:
>
>    (1) CREATE SEQUENCE, for serial column
>    (2) CREATE TABLE, skipped if table exists with same name
>    (3) ALTER SEQUENCE OWNED BY, associate sequence with table
>
> This error occurs in (3) because process_owned_by() misunderstand
> that existing table is new owner, but it's a foreign server and
> shouldn't be used as owner.  So same error occurs if the existing
> relation was an index or a sequence.

I see what you mean, so the error is unrelated to any foreign table
support and applies to any database object that's not a regular table.
Do we still want this behaviour for foreign tables, or should they be
made an exception as they are a type of table? Although to be fair, I
can't see the use case for it.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Apr 1, 2011 at 5:13 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 1 April 2011 12:57, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> NOT NULL constraint on foreign table is just declaration and can't
>> force data integrity.  And I noticed that CREATE FOREIGN TABLE
>> document doesn't mention that serial and bigserial can't be used in
>> foreign table.  Please see foreign_table_doc.patch for this fix.
>
> I'd be inclined to generalise it to say that default values can't be
> used on a foreign table, and then say that as a result, serial and
> bigserial can't be used.

+1.

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

On Fri, Apr 1, 2011 at 1:29 AM, Shigeru HANADA
<hanada(at)metrosystems(dot)co(dot)jp> wrote:
> In addition to the 2nd GRANT above, "GRANT SELECT (colour) ON stuff TO
> user_a" (omitting TABLE) will succeed too because parser assumes that
> the target object is a regular table if object type was TABLE or
> omitted. This inconsistent behavior would be an oversight and need to
> be fixed.

+1.

> How about to drop "GRANT xxx ON FOREIGN TABLE foo" syntax support and
> use "GRANT xxx ON [TABLE] foo" for foreign tables?  ISTM that "ON
> FOREIGN TABLE" specification is useless because possible privilege
> type would be same as TABLE.

-1. We should be consistent about treating foreign tables as their
own object type - and the possible privilege types are NOT the same -
only SELECT is supported.

> Probabry we should mention in GRANT documents that ALL TABLES
> IN SCHEMA is considered to include foreign tables.

Or else change the behavior so that it doesn't, which would probably be my vote.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Apr 1, 2011 at 5:13 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 1 April 2011 12:57, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>>> NOT NULL constraint on foreign table is just declaration and can't
>>> force data integrity. And I noticed that CREATE FOREIGN TABLE
>>> document doesn't mention that serial and bigserial can't be used in
>>> foreign table. Please see foreign_table_doc.patch for this fix.

>> I'd be inclined to generalise it to say that default values can't be
>> used on a foreign table, and then say that as a result, serial and
>> bigserial can't be used.

> +1.

Why is this a documentation issue and not a code issue? IMO we should
flat out reject both NOT NULL and DEFAULT declarations on foreign
tables, until such time as we're prepared to do something useful with
them. Reasons:

1. Accepting non-functional constraint declarations is something we've
been heard to ridicule mysql for. With good reason.

2. It probably won't be too long before the planner makes optimization
decisions that assume NOT NULL declarations to be truthful. When that
day comes, I don't want to be seeing an exception for foreign tables in
that logic.

3. When we do get around to making it actually work, we will have a
backwards-compatibility problem if prior versions accepted the
declaration but treated it as a no-op.

regards, tom lane

On 15 April 2011 04:26, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Why is this a documentation issue and not a code issue?  IMO we should
> flat out reject both NOT NULL and DEFAULT declarations on foreign
> tables, until such time as we're prepared to do something useful with
> them.  Reasons:

If the removal the redundant declarations is do-able for this release,
that would be preferable. And if that change is to happen, I guess it
has to start happening immediately, letting the pgAdmin guys know too.

> 1. Accepting non-functional constraint declarations is something we've
> been heard to ridicule mysql for.  With good reason.

Fair point.

> 2. It probably won't be too long before the planner makes optimization
> decisions that assume NOT NULL declarations to be truthful.  When that
> day comes, I don't want to be seeing an exception for foreign tables in
> that logic.

Makes sense.

> 3. When we do get around to making it actually work, we will have a
> backwards-compatibility problem if prior versions accepted the
> declaration but treated it as a no-op.

Probably the most important point.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 14, 2011 at 8:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 1. Accepting non-functional constraint declarations is something we've
> been heard to ridicule mysql for.  With good reason.
>
> 2. It probably won't be too long before the planner makes optimization
> decisions that assume NOT NULL declarations to be truthful.  When that
> day comes, I don't want to be seeing an exception for foreign tables in
> that logic.
>
> 3. When we do get around to making it actually work, we will have a
> backwards-compatibility problem if prior versions accepted the
> declaration but treated it as a no-op.

The planner *already* makes optimization decisions that assume NOT
NULL declarations are truthful (see: left join reordering). That's
why we have them for foreign tables, and why we eventually will need
constraints as well.

Of course, we have no guarantee that the data on the foreign side
matches those constraints. But we don't have any guarantee that it
matches the data type declarations, either. We could insist that
every column must be of type text and allow NULLs, but that seems like
it would be losing rather a lot of the functionality. The point of a
datatype declaration, or a NOT NULL declaration, or any other such
thing with respect to foreign tables is that the user is making an
assertion about what the data looks like, hopefully with some
cooperation from the FDW. It's ridiculous to suppose that we will
really be able to control anything at all about the data on the
foreign side, even the number of columns. But our job is to shove
whatever information is present into the schema the user has
specified, or throw an error.

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

(2011/04/15 3:43), Robert Haas wrote:
> On Fri, Apr 1, 2011 at 1:29 AM, Shigeru HANADA
> <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> In addition to the 2nd GRANT above, "GRANT SELECT (colour) ON stuff TO
>> user_a" (omitting TABLE) will succeed too because parser assumes that
>> the target object is a regular table if object type was TABLE or
>> omitted. This inconsistent behavior would be an oversight and need to
>> be fixed.
>
> +1.
>
>> How about to drop "GRANT xxx ON FOREIGN TABLE foo" syntax support and
>> use "GRANT xxx ON [TABLE] foo" for foreign tables? ISTM that "ON
>> FOREIGN TABLE" specification is useless because possible privilege
>> type would be same as TABLE.
>
> -1. We should be consistent about treating foreign tables as their
> own object type - and the possible privilege types are NOT the same -
> only SELECT is supported.
>
>> Probabry we should mention in GRANT documents that ALL TABLES
>> IN SCHEMA is considered to include foreign tables.
>
> Or else change the behavior so that it doesn't, which would probably be my vote.

Thanks for the comments. I agree that foreign table is a different
object type from regular table or view in the context of ACL.

Attached patch implements along specifications below. It also includes
documents and regression tests. Some of regression tests might be
redundant and removable.

1) "GRANT privilege [(column_list)] ON [TABLE] TO role" also work for
foreign tables as well as regular tables, if specified privilege was
SELECT. This might seem little inconsistent but I feel natural to use
this syntax for SELECT-able objects. Anyway, such usage can be disabled
with trivial fix.

2) "GRANT privilege [(column_list)] ON FOREIGN TABLE table TO role"
works only for foreign tables, and accepts only SELECT as privilege.

3) "GRANT privilege ON ALL TABLES IN SCHEMA schema TO role" doesn't
affect any foreign table in the schema.

4) "GRANT privilege [(column_list)] ON ALL FOREIGN TABLES IN SCHEMA
schema TO role" works for all foreign tables in the schema, but not
affect any regular table, view or sequence in the schema.

BTW, I noticed some issues about ACL below. Some of them might have to
be fixed in future.

a) "GRANT privilege(column_list) ON ALL TABLES IN SCHEMA schema" works
fine if all of the tables in the schema have all of listed columns. It
is not documented, and might be unintentional.

b) ALTER DEFAULT PRIVILEGE doesn't support foreign tables.

c) Currently SELECT privilege allow user to retrieve contents of the
foreign table, but this is different from SQL/MED Standard. Should this
difference be mentioned in GRANT document?
> [4.14.1 Privileges]
> NOTE 9 — Privileges granted on foreign tables are not privileges to
> use the data constituting foreign tables, but privileges to use the
> definitions of the foreign tables. The privileges to access the data
> constituting the foreign tables are enforced by the foreign server,
> based on the user mapping. Consequently, a request by an SQL-client
> to access external data may raise exceptions.

Regards,
--
Shigeru Hanada

Shigeru Hanada <hanada(at)metrosystems(dot)co(dot)jp> writes:
> Attached patch implements along specifications below. It also includes
> documents and regression tests. Some of regression tests might be
> redundant and removable.

> 1) "GRANT privilege [(column_list)] ON [TABLE] TO role" also work for
> foreign tables as well as regular tables, if specified privilege was
> SELECT. This might seem little inconsistent but I feel natural to use
> this syntax for SELECT-able objects. Anyway, such usage can be disabled
> with trivial fix.

It seems really seriously inconsistent to do that at the same time that
you make other forms of GRANT treat foreign tables as a separate class
of object. I think if they're going to be a separate class of object,
they should be separate, full stop. Making them just mostly separate
will confuse people no end.

regards, tom lane

On Wed, Apr 20, 2011 at 9:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Shigeru Hanada <hanada(at)metrosystems(dot)co(dot)jp> writes:
>> Attached patch implements along specifications below.  It also includes
>> documents and regression tests.  Some of regression tests might be
>> redundant and removable.
>
>> 1) "GRANT privilege [(column_list)] ON [TABLE] TO role" also work for
>> foreign tables as well as regular tables, if specified privilege was
>> SELECT.  This might seem little inconsistent but I feel natural to use
>> this syntax for SELECT-able objects.  Anyway, such usage can be disabled
>> with trivial fix.
>
> It seems really seriously inconsistent to do that at the same time that
> you make other forms of GRANT treat foreign tables as a separate class
> of object.  I think if they're going to be a separate class of object,
> they should be separate, full stop.  Making them just mostly separate
> will confuse people no end.

I agree.

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

On Wed, Apr 20, 2011 at 11:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Apr 20, 2011 at 9:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Shigeru Hanada <hanada(at)metrosystems(dot)co(dot)jp> writes:
>>> Attached patch implements along specifications below.  It also includes
>>> documents and regression tests.  Some of regression tests might be
>>> redundant and removable.
>>
>>> 1) "GRANT privilege [(column_list)] ON [TABLE] TO role" also work for
>>> foreign tables as well as regular tables, if specified privilege was
>>> SELECT.  This might seem little inconsistent but I feel natural to use
>>> this syntax for SELECT-able objects.  Anyway, such usage can be disabled
>>> with trivial fix.
>>
>> It seems really seriously inconsistent to do that at the same time that
>> you make other forms of GRANT treat foreign tables as a separate class
>> of object.  I think if they're going to be a separate class of object,
>> they should be separate, full stop.  Making them just mostly separate
>> will confuse people no end.
>
> I agree.

Hmm, it appears we had some pre-existing inconsistency here, because
ALL TABLES IN <schema> currently includes views. That's weird, but
it'll be even more weird if we adopt the approach suggested by this
patch, which creates ALL FOREIGN TABLES IN <schema> but allows ALL
TABLES IN <schema> to go on including views. Maybe there is an
argument for having ALL {TABLES|VIEWS|FOREIGN TABLES} IN <schema> - or
maybe there isn't - but having two out of the three of them doesn't do
anything for me. For now I think we should go with the path of least
resistance and just document that ALL TABLES IN <schema> now includes
not only views but also foreign tables.

Putting that together with the comments already made upthread, the
only behavior changes I think we should make here are:

- Add GRANT privilege [(column_list)] ON FOREIGN TABLE table TO role.
- Require that the argument to GRANT privilege [(column_list)] ON
TABLE TO role be an ordinary table, not a foreign table.

That looks like enough to make foreign table handling consistent with
what we're already doing.

Barring objections, I'll go make that happen.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Hmm, it appears we had some pre-existing inconsistency here, because
> ALL TABLES IN <schema> currently includes views. That's weird, but
> it'll be even more weird if we adopt the approach suggested by this
> patch, which creates ALL FOREIGN TABLES IN <schema> but allows ALL
> TABLES IN <schema> to go on including views. Maybe there is an
> argument for having ALL {TABLES|VIEWS|FOREIGN TABLES} IN <schema> - or
> maybe there isn't - but having two out of the three of them doesn't do
> anything for me.

Yeah, that's a fair point. Another issue is that eventually foreign
tables will probably have some update capability, so designing GRANT
on the assumption that only SELECT should be allowed is a mistake.
In fact, I'd argue that GRANT ought not be enforcing such an assumption
even today, especially if it leads to asymmetry there. Let somebody
GRANT UPDATE if they want to --- there's no need to throw an error until
the update operation is actually tried.

> Putting that together with the comments already made upthread, the
> only behavior changes I think we should make here are:

> - Add GRANT privilege [(column_list)] ON FOREIGN TABLE table TO role.
> - Require that the argument to GRANT privilege [(column_list)] ON
> TABLE TO role be an ordinary table, not a foreign table.

I think this might be going in the wrong direction given the above
thoughts. At the very least you're going to have to make sure the
prohibition is easily reversible.

regards, tom lane

On Mon, Apr 25, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Hmm, it appears we had some pre-existing inconsistency here, because
>> ALL TABLES IN <schema> currently includes views.  That's weird, but
>> it'll be even more weird if we adopt the approach suggested by this
>> patch, which creates ALL FOREIGN TABLES IN <schema> but allows ALL
>> TABLES IN <schema> to go on including views.  Maybe there is an
>> argument for having ALL {TABLES|VIEWS|FOREIGN TABLES} IN <schema> - or
>> maybe there isn't - but having two out of the three of them doesn't do
>> anything for me.
>
> Yeah, that's a fair point.  Another issue is that eventually foreign
> tables will probably have some update capability, so designing GRANT
> on the assumption that only SELECT should be allowed is a mistake.
> In fact, I'd argue that GRANT ought not be enforcing such an assumption
> even today, especially if it leads to asymmetry there.  Let somebody
> GRANT UPDATE if they want to --- there's no need to throw an error until
> the update operation is actually tried.
>
>> Putting that together with the comments already made upthread, the
>> only behavior changes I think we should make here are:
>
>> - Add GRANT privilege [(column_list)] ON FOREIGN TABLE table TO role.
>> - Require that the argument to GRANT privilege [(column_list)] ON
>> TABLE TO role be an ordinary table, not a foreign table.
>
> I think this might be going in the wrong direction given the above
> thoughts.  At the very least you're going to have to make sure the
> prohibition is easily reversible.

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.

So now I think this is just a documentation bug.

Do you agree?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Apr 25, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

regards, tom lane

On mån, 2011-04-25 at 13:35 -0400, Robert Haas wrote:
> Hmm, it appears we had some pre-existing inconsistency here, because
> ALL TABLES IN <schema> currently includes views.

Which makes sense because you use GRANT ... ON TABLE to grant privileges
to views.

> That's weird, but
> it'll be even more weird if we adopt the approach suggested by this
> patch, which creates ALL FOREIGN TABLES IN <schema> but allows ALL
> TABLES IN <schema> to go on including views. Maybe there is an
> argument for having ALL {TABLES|VIEWS|FOREIGN TABLES} IN <schema> - or
> maybe there isn't - but having two out of the three of them doesn't do
> anything for me. For now I think we should go with the path of least
> resistance and just document that ALL TABLES IN <schema> now includes
> not only views but also foreign tables.

Yes.

> Putting that together with the comments already made upthread, the
> only behavior changes I think we should make here are:
>
> - Add GRANT privilege [(column_list)] ON FOREIGN TABLE table TO role.
> - Require that the argument to GRANT privilege [(column_list)] ON
> TABLE TO role be an ordinary table, not a foreign table.

But that would be contrary to the SQL standard. The current behavior is
fine, AFAICT.

On Mon, Apr 25, 2011 at 2:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Apr 25, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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("foreign table \"%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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> ... 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).

Well, the relevant point here is that there's little or no likelihood
that we'll ever care to support direct UPDATE on sequences. This is
exactly not the case for foreign tables. So I would argue that GRANT
should handle them like views; certainly not be even more strict than
it is for sequences.

IOW, yeah, let's drop these two checks.

regards, tom lane

On Mon, Apr 25, 2011 at 7:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

Actually I vaguely remember some earlier pass through this code to
make it more consistent. IIRC we previously had some commands that
could only be done through ALTER TABLE even for things like sequences
and views and other commands that had corresponding ALTER VIEW
commands. I'll try to see if I can dig up the messages on the topic.

--
greg

On Mon, Apr 25, 2011 at 2:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> ... 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).
>
> Well, the relevant point here is that there's little or no likelihood
> that we'll ever care to support direct UPDATE on sequences.  This is
> exactly not the case for foreign tables.  So I would argue that GRANT
> should handle them like views; certainly not be even more strict than
> it is for sequences.
>
> IOW, yeah, let's drop these two checks.

OK. Turned out a little more cleanup was needed to make this all the
way consistent with how we handle views; I have now done that.

<pauses to listen for screaming noises>

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

(2011/04/26 5:42), Robert Haas wrote:
> OK. Turned out a little more cleanup was needed to make this all the
> way consistent with how we handle views; I have now done that.

I noticed that some fixes would be needed for consistency about foreign
table privileges. Attached patch includes fixes below:

1) psql doesn't complete FOREIGN TABLE after GRANT/REVOKE.
2) pg_dump uses GRANT .. ON TABLE for foreign tables, instead of ON
FOREIGN TABLE.
3) GRANT document mentions that ALL TABLES includes foreign tables too.
4) Rows of information_schema.foreign_tables/foreign_table_options are
visible to users who have any privileges on the foreign table.

Regards,
--
Shigeru Hanada

2011/5/11 Shigeru Hanada <hanada(at)metrosystems(dot)co(dot)jp>:
> (2011/04/26 5:42), Robert Haas wrote:
>> OK.  Turned out a little more cleanup was needed to make this all the
>> way consistent with how we handle views; I have now done that.
>
> I noticed that some fixes would be needed for consistency about foreign
> table privileges. Attached patch includes fixes below:
>
> 1) psql doesn't complete FOREIGN TABLE after GRANT/REVOKE.
> 2) pg_dump uses GRANT .. ON TABLE for foreign tables, instead of ON
> FOREIGN TABLE.
> 3) GRANT document mentions that ALL TABLES includes foreign tables too.
> 4) Rows of information_schema.foreign_tables/foreign_table_options are
> visible to users who have any privileges on the foreign table.

Thanks; I'm embarrassed I didn't notice those things myself. I've
committed this patch with very slight adjustment.

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