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@linux.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: filepostgres=# 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 tableSince 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@linux.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: filepostgres=# 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
Attachments:
20110401_column_privs.patchapplication/octet-stream; name=20110401_column_privs.patchDownload
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 72ecc45..1114d98 100644
*** a/doc/src/sgml/ref/grant.sgml
--- b/doc/src/sgml/ref/grant.sgml
*************** GRANT <replaceable class="PARAMETER">rol
*** 81,87 ****
<para>
The <command>GRANT</command> command has two basic variants: one
that grants privileges on a database object (table, column, view, sequence,
! database, foreign-data wrapper, foreign server, function,
procedural language, schema, or tablespace), and one that grants
membership in a role. These variants are similar in many ways, but
they are different enough to be described separately.
--- 81,87 ----
<para>
The <command>GRANT</command> command has two basic variants: one
that grants privileges on a database object (table, column, view, sequence,
! foreign table, database, foreign-data wrapper, foreign server, function,
procedural language, schema, or tablespace), and one that grants
membership in a role. These variants are similar in many ways, but
they are different enough to be described separately.
*************** GRANT <replaceable class="PARAMETER">rol
*** 101,107 ****
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
only for tables, sequences, and functions (but note that <literal>ALL
! TABLES</> is considered to include views).
</para>
<para>
--- 101,107 ----
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
only for tables, sequences, and functions (but note that <literal>ALL
! TABLES</> is considered to include views and foreign tables).
</para>
<para>
*************** GRANT <replaceable class="PARAMETER">rol
*** 166,172 ****
<para>
Allows <xref linkend="sql-select"> from
any column, or the specific columns listed, of the specified table,
! view, or sequence.
Also allows the use of
<xref linkend="sql-copy"> TO.
This privilege is also needed to reference existing column values in
--- 166,172 ----
<para>
Allows <xref linkend="sql-select"> from
any column, or the specific columns listed, of the specified table,
! view, sequence, or foreign table.
Also allows the use of
<xref linkend="sql-copy"> TO.
This privilege is also needed to reference existing column values in
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 48fa6d4..3532cfa 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** ExecGrant_Relation(InternalGrant *istmt)
*** 1702,1715 ****
errmsg("\"%s\" is not a sequence",
NameStr(pg_class_tuple->relname))));
- /* Used GRANT FOREIGN TABLE on a non-foreign-table? */
- if (istmt->objtype == ACL_OBJECT_FOREIGN_TABLE &&
- pg_class_tuple->relkind != RELKIND_FOREIGN_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a foreign table",
- NameStr(pg_class_tuple->relname))));
-
/* Adjust the default permissions based on object type */
if (istmt->all_privs && istmt->privileges == ACL_NO_RIGHTS)
{
--- 1702,1707 ----
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 27fdcca..720903b 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** privilege_target:
*** 5388,5401 ****
n->objs = $3;
$$ = n;
}
- | FOREIGN TABLE qualified_name_list
- {
- PrivTarget *n = (PrivTarget *) palloc(sizeof(PrivTarget));
- n->targtype = ACL_TARGET_OBJECT;
- n->objtype = ACL_OBJECT_FOREIGN_TABLE;
- n->objs = $3;
- $$ = n;
- }
| FUNCTION function_with_argtypes_list
{
PrivTarget *n = (PrivTarget *) palloc(sizeof(PrivTarget));
--- 5388,5393 ----
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index e72c5b9..5412020 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** psql_completion(char *text, int start, i
*** 2216,2222 ****
" UNION SELECT 'DATABASE'"
" UNION SELECT 'FOREIGN DATA WRAPPER'"
" UNION SELECT 'FOREIGN SERVER'"
- " UNION SELECT 'FOREIGN TABLE'"
" UNION SELECT 'FUNCTION'"
" UNION SELECT 'LANGUAGE'"
" UNION SELECT 'LARGE OBJECT'"
--- 2216,2221 ----
*************** psql_completion(char *text, int start, i
*** 2228,2234 ****
pg_strcasecmp(prev_wd, "FOREIGN") == 0)
{
static const char *const list_privilege_foreign[] =
! {"DATA WRAPPER", "SERVER", "TABLE", NULL};
COMPLETE_WITH_LIST(list_privilege_foreign);
}
--- 2227,2233 ----
pg_strcasecmp(prev_wd, "FOREIGN") == 0)
{
static const char *const list_privilege_foreign[] =
! {"DATA WRAPPER", "SERVER", NULL};
COMPLETE_WITH_LIST(list_privilege_foreign);
}
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index a747334..2875773 100644
*** a/src/test/regress/expected/foreign_data.out
--- b/src/test/regress/expected/foreign_data.out
*************** ERROR: foreign-data wrapper "dummy" has
*** 674,679 ****
--- 674,689 ----
EXPLAIN SELECT * FROM ft1; -- ERROR
ERROR: foreign-data wrapper "dummy" has no handler
-- ALTER FOREIGN TABLE
+ GRANT SELECT(c1) ON TABLE ft1 TO regress_test_role;
+ GRANT ALL ON TABLE ft1 TO regress_test_role;
+ \dp ft1
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ --------+------+---------------+---------------------------------------+-----------------------------------------
+ public | ft1 | foreign table | foreign_data_user=r/foreign_data_user+| c1: +
+ | | | regress_test_role=r/foreign_data_user | regress_test_role=r/foreign_data_user
+ (1 row)
+
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
COMMENT ON COLUMN ft1.c1 IS 'foreign column';
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 3f39785..04b3ad5 100644
*** a/src/test/regress/sql/foreign_data.sql
--- b/src/test/regress/sql/foreign_data.sql
*************** SELECT * FROM ft1;
*** 275,280 ****
--- 275,283 ----
EXPLAIN SELECT * FROM ft1; -- ERROR
-- ALTER FOREIGN TABLE
+ GRANT SELECT(c1) ON TABLE ft1 TO regress_test_role;
+ GRANT ALL ON TABLE ft1 TO regress_test_role;
+ \dp ft1
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
COMMENT ON COLUMN ft1.c1 IS 'foreign column';
On Fri, 1 Apr 2011 01:24:20 +0100
Thom Brown <thom@linux.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 tableThe 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
Attachments:
20110401_foreign_table_doc.patchapplication/octet-stream; name=20110401_foreign_table_doc.patchDownload
diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
index ad91072..93f910f 100644
*** a/doc/src/sgml/ref/create_foreign_table.sgml
--- b/doc/src/sgml/ref/create_foreign_table.sgml
*************** CREATE FOREIGN TABLE [ IF NOT EXISTS ] <
*** 94,100 ****
<listitem>
<para>
The data type of the column. This can include array
! specifiers. For more information on the data types supported by
<productname>PostgreSQL</productname>, refer to <xref
linkend="datatype">.
</para>
--- 94,101 ----
<listitem>
<para>
The data type of the column. This can include array
! specifiers, but serial and bigserial can't be used for foreign table.
! For more information on the data types supported by
<productname>PostgreSQL</productname>, refer to <xref
linkend="datatype">.
</para>
*************** CREATE FOREIGN TABLE [ IF NOT EXISTS ] <
*** 107,112 ****
--- 108,119 ----
<para>
The column is not allowed to contain null values.
</para>
+ <para>
+ Note that NOT NULL constraint on a foreign table column is just
+ a declaration, and doesn't provide any mechanism to prevent null values
+ to be returned. Foreign data is not checked even on creation of a
+ foreign table, so data provider must guarantee the integrity of data.
+ </para>
</listitem>
</varlistentry>
On 1 April 2011 12:57, Shigeru HANADA <hanada@metrosystems.co.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 tableThis 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@linux.com> wrote:
On 1 April 2011 12:57, Shigeru HANADA <hanada@metrosystems.co.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@metrosystems.co.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@gmail.com> writes:
On Fri, Apr 1, 2011 at 5:13 AM, Thom Brown <thom@linux.com> wrote:
On 1 April 2011 12:57, Shigeru HANADA <hanada@metrosystems.co.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@sss.pgh.pa.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@sss.pgh.pa.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@metrosystems.co.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
Attachments:
foreign_table_privs_v1.patchtext/plain; name=foreign_table_privs_v1.patchDownload
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 72ecc45..3ad9fab 100644
*** a/doc/src/sgml/ref/grant.sgml
--- b/doc/src/sgml/ref/grant.sgml
*************** GRANT { { SELECT | INSERT | UPDATE | REF
*** 32,37 ****
--- 32,46 ----
ON [ TABLE ] <replaceable class="PARAMETER">table_name</replaceable> [, ...]
TO { [ GROUP ] <replaceable class="PARAMETER">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+ GRANT { SELECT | ALL [ PRIVILEGES ] }
+ ON { FOREIGN TABLE <replaceable class="PARAMETER">foreign_table_name</replaceable> [, ...]
+ | ALL FOREIGN TABLES IN SCHEMA <replaceable class="PARAMETER">schema_name</replaceable> [, ...] }
+ TO { [ GROUP ] <replaceable class="PARAMETER">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
+ GRANT { SELECT | ALL [ PRIVILEGES ] } ( <replaceable class="PARAMETER">column</replaceable> [, ...] )
+ ON FOREIGN TABLE <replaceable class="PARAMETER">foreign_table_name</replaceable> [, ...]
+ TO { [ GROUP ] <replaceable class="PARAMETER">role_name</replaceable> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
GRANT { { USAGE | SELECT | UPDATE }
[, ...] | ALL [ PRIVILEGES ] }
ON { SEQUENCE <replaceable class="PARAMETER">sequence_name</replaceable> [, ...]
*************** GRANT <replaceable class="PARAMETER">rol
*** 81,87 ****
<para>
The <command>GRANT</command> command has two basic variants: one
that grants privileges on a database object (table, column, view, sequence,
! database, foreign-data wrapper, foreign server, function,
procedural language, schema, or tablespace), and one that grants
membership in a role. These variants are similar in many ways, but
they are different enough to be described separately.
--- 90,96 ----
<para>
The <command>GRANT</command> command has two basic variants: one
that grants privileges on a database object (table, column, view, sequence,
! foreign table, database, foreign-data wrapper, foreign server, function,
procedural language, schema, or tablespace), and one that grants
membership in a role. These variants are similar in many ways, but
they are different enough to be described separately.
*************** GRANT <replaceable class="PARAMETER">rol
*** 100,107 ****
<para>
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
! only for tables, sequences, and functions (but note that <literal>ALL
! TABLES</> is considered to include views).
</para>
<para>
--- 109,116 ----
<para>
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
! only for tables, sequences, functions and foreign tables (but note that
! <literal>ALL TABLES</> is considered to include views).
</para>
<para>
diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
index 204c986..70702bd 100644
*** a/doc/src/sgml/ref/revoke.sgml
--- b/doc/src/sgml/ref/revoke.sgml
*************** REVOKE [ GRANT OPTION FOR ]
*** 37,42 ****
--- 37,56 ----
[ CASCADE | RESTRICT ]
REVOKE [ GRANT OPTION FOR ]
+ { SELECT | ALL [ PRIVILEGES ] }
+ ON { FOREIGN TABLE <replaceable class="PARAMETER">foreign_table_name</replaceable> [, ...]
+ | ALL FOREIGN TABLES IN SCHEMA <replaceable>schema_name</replaceable> [, ...] }
+ FROM { [ GROUP ] <replaceable class="PARAMETER">role_name</replaceable> | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
+ { SELECT ( <replaceable class="PARAMETER">column</replaceable> [, ...] )
+ | ALL [ PRIVILEGES ] ( <replaceable class="PARAMETER">column</replaceable> [, ...] ) }
+ ON FOREIGN TABLE <replaceable class="PARAMETER">foreign_table_name</replaceable> [, ...]
+ FROM { [ GROUP ] <replaceable class="PARAMETER">role_name</replaceable> | PUBLIC } [, ...]
+ [ CASCADE | RESTRICT ]
+
+ REVOKE [ GRANT OPTION FOR ]
{ { USAGE | SELECT | UPDATE }
[, ...] | ALL [ PRIVILEGES ] }
ON { SEQUENCE <replaceable class="PARAMETER">sequence_name</replaceable> [, ...]
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 693b634..5aa9977 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** ExecuteGrantStmt(GrantStmt *stmt)
*** 518,524 ****
*/
if (privnode->cols)
{
! if (stmt->objtype != ACL_OBJECT_RELATION)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("column privileges are only valid for relations")));
--- 518,525 ----
*/
if (privnode->cols)
{
! if (stmt->objtype != ACL_OBJECT_RELATION &&
! stmt->objtype != ACL_OBJECT_FOREIGN_TABLE)
ereport(ERROR,
(errcode(ERRCODE_INVALID_GRANT_OPERATION),
errmsg("column privileges are only valid for relations")));
*************** objectsInSchemaToOids(GrantObjectType ob
*** 729,746 ****
switch (objtype)
{
case ACL_OBJECT_RELATION:
! /* Process regular tables, views and foreign tables */
objs = getRelationsInNamespace(namespaceId, RELKIND_RELATION);
objects = list_concat(objects, objs);
objs = getRelationsInNamespace(namespaceId, RELKIND_VIEW);
objects = list_concat(objects, objs);
- objs = getRelationsInNamespace(namespaceId, RELKIND_FOREIGN_TABLE);
- objects = list_concat(objects, objs);
break;
case ACL_OBJECT_SEQUENCE:
objs = getRelationsInNamespace(namespaceId, RELKIND_SEQUENCE);
objects = list_concat(objects, objs);
break;
case ACL_OBJECT_FUNCTION:
{
ScanKeyData key[1];
--- 730,750 ----
switch (objtype)
{
case ACL_OBJECT_RELATION:
! /* Process regular tables and views */
objs = getRelationsInNamespace(namespaceId, RELKIND_RELATION);
objects = list_concat(objects, objs);
objs = getRelationsInNamespace(namespaceId, RELKIND_VIEW);
objects = list_concat(objects, objs);
break;
case ACL_OBJECT_SEQUENCE:
objs = getRelationsInNamespace(namespaceId, RELKIND_SEQUENCE);
objects = list_concat(objects, objs);
break;
+ case ACL_OBJECT_FOREIGN_TABLE:
+ objs = getRelationsInNamespace(namespaceId,
+ RELKIND_FOREIGN_TABLE);
+ objects = list_concat(objects, objs);
+ break;
case ACL_OBJECT_FUNCTION:
{
ScanKeyData key[1];
*************** ExecGrant_Relation(InternalGrant *istmt)
*** 1938,1944 ****
AccessPriv *col_privs = (AccessPriv *) lfirst(cell_colprivs);
if (col_privs->priv_name == NULL)
! this_privileges = ACL_ALL_RIGHTS_COLUMN;
else
this_privileges = string_to_privilege(col_privs->priv_name);
--- 1942,1957 ----
AccessPriv *col_privs = (AccessPriv *) lfirst(cell_colprivs);
if (col_privs->priv_name == NULL)
! {
! /*
! * Columns of a foreign table accept same privilege as foreign
! * table itself.
! */
! if (pg_class_tuple->relkind == RELKIND_FOREIGN_TABLE)
! this_privileges = ACL_ALL_RIGHTS_FOREIGN_TABLE;
! else
! this_privileges = ACL_ALL_RIGHTS_COLUMN;
! }
else
this_privileges = string_to_privilege(col_privs->priv_name);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a22ab66..5ad69e0 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** privilege_target:
*** 5463,5468 ****
--- 5463,5476 ----
n->objs = $5;
$$ = n;
}
+ | ALL FOREIGN TABLES IN_P SCHEMA name_list
+ {
+ PrivTarget *n = (PrivTarget *) palloc(sizeof(PrivTarget));
+ n->targtype = ACL_TARGET_ALL_IN_SCHEMA;
+ n->objtype = ACL_OBJECT_FOREIGN_TABLE;
+ n->objs = $6;
+ $$ = n;
+ }
| ALL FUNCTIONS IN_P SCHEMA name_list
{
PrivTarget *n = (PrivTarget *) palloc(sizeof(PrivTarget));
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index c05bcab..4e284a5 100644
*** a/src/test/regress/expected/foreign_data.out
--- b/src/test/regress/expected/foreign_data.out
*************** SELECT * FROM ft1;
*** 675,680 ****
--- 675,811 ----
ERROR: foreign-data wrapper "dummy" has no handler
EXPLAIN SELECT * FROM ft1; -- ERROR
ERROR: foreign-data wrapper "dummy" has no handler
+ -- GRANT FOREIGN TABLE
+ GRANT SELECT ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT SELECT (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT INSERT ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type INSERT for foreign table
+ GRANT INSERT (c1) ON FOREIGN TABLE ft1 TO foreign_data_user; -- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT UPDATE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type UPDATE for foreign table
+ GRANT UPDATE (c1) ON FOREIGN TABLE ft1 TO foreign_data_user; -- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT DELETE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type DELETE for foreign table
+ GRANT TRUNCATE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type TRUNCATE for foreign table
+ GRANT REFERENCES ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type REFERENCES for foreign table
+ GRANT REFERENCES (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;-- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT TRIGGER ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type TRIGGER for foreign table
+ GRANT USAGE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type USAGE for foreign table
+ GRANT EXECUTE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type EXECUTE for foreign table
+ -- omitting FOREIGN TABLE
+ GRANT SELECT ON ft1 TO foreign_data_user;
+ GRANT SELECT (c1) ON ft1 TO foreign_data_user;
+ GRANT INSERT ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT INSERT (c1) ON ft1 TO foreign_data_user; -- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT UPDATE ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT UPDATE (c1) ON ft1 TO foreign_data_user; -- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT DELETE ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT TRUNCATE ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT REFERENCES ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT REFERENCES (c1) ON ft1 TO foreign_data_user ;-- WARNING
+ WARNING: foreign table "ft1" only supports SELECT column privileges
+ GRANT TRIGGER ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT USAGE ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: foreign table "ft1" only supports SELECT privileges
+ GRANT EXECUTE ON ft1 TO foreign_data_user; -- ERROR
+ ERROR: invalid privilege type EXECUTE for relation
+ \dp ft1
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ --------+------+---------------+---------------------------------------+-----------------------------------------
+ public | ft1 | foreign table | foreign_data_user=r/foreign_data_user | c1: +
+ | | | | foreign_data_user=r/foreign_data_user
+ (1 row)
+
+ REVOKE SELECT ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE SELECT (c1) ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE INSERT ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type INSERT for foreign table
+ REVOKE UPDATE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type UPDATE for foreign table
+ REVOKE DELETE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type DELETE for foreign table
+ REVOKE TRUNCATE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type TRUNCATE for foreign table
+ REVOKE REFERENCES ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type REFERENCES for foreign table
+ REVOKE TRIGGER ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type TRIGGER for foreign table
+ REVOKE USAGE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type USAGE for foreign table
+ REVOKE EXECUTE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ ERROR: invalid privilege type EXECUTE for foreign table
+ \dp ft1
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ --------+------+---------------+-------------------+--------------------------
+ public | ft1 | foreign table | |
+ (1 row)
+
+ -- GRANT/REVOKE ALL
+ GRANT ALL ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT ALL (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;
+ \dp ft1
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ --------+------+---------------+---------------------------------------+-----------------------------------------
+ public | ft1 | foreign table | foreign_data_user=r/foreign_data_user | c1: +
+ | | | | foreign_data_user=r/foreign_data_user
+ (1 row)
+
+ REVOKE ALL ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE ALL (c1) ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ \dp ft1
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ --------+------+---------------+-------------------+--------------------------
+ public | ft1 | foreign table | |
+ (1 row)
+
+ -- GRANT ALL FOREIGN TABLES IN SCHEMA
+ ALTER FOREIGN TABLE ft1 SET SCHEMA foreign_schema;
+ CREATE FOREIGN TABLE foreign_schema.ft2 (c1 int) SERVER sc;
+ GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ GRANT SELECT (c1) ON ALL FOREIGN TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ GRANT UPDATE ON ALL TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ \dp foreign_schema.ft*
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ ----------------+------+---------------+---------------------------------------+-----------------------------------------
+ foreign_schema | ft1 | foreign table | foreign_data_user=r/foreign_data_user | c1: +
+ | | | | foreign_data_user=r/foreign_data_user
+ foreign_schema | ft2 | foreign table | foreign_data_user=r/foreign_data_user | c1: +
+ | | | | foreign_data_user=r/foreign_data_user
+ (2 rows)
+
+ REVOKE SELECT ON ALL FOREIGN TABLES IN SCHEMA foreign_schema FROM foreign_data_user;
+ REVOKE SELECT (c1) ON ALL FOREIGN TABLES IN SCHEMA foreign_schema FROM foreign_data_user;
+ \dp foreign_schema.ft*
+ Access privileges
+ Schema | Name | Type | Access privileges | Column access privileges
+ ----------------+------+---------------+-------------------+--------------------------
+ foreign_schema | ft1 | foreign table | |
+ foreign_schema | ft2 | foreign table | |
+ (2 rows)
+
+ ALTER FOREIGN TABLE foreign_schema.ft1 SET SCHEMA public;
+ DROP FOREIGN TABLE foreign_schema.ft2;
-- ALTER FOREIGN TABLE
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 0d12b98..ddd1cf0 100644
*** a/src/test/regress/sql/foreign_data.sql
--- b/src/test/regress/sql/foreign_data.sql
*************** CREATE INDEX id_ft1_c2 ON ft1 (c2);
*** 276,281 ****
--- 276,341 ----
SELECT * FROM ft1; -- ERROR
EXPLAIN SELECT * FROM ft1; -- ERROR
+ -- GRANT FOREIGN TABLE
+ GRANT SELECT ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT SELECT (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT INSERT ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT INSERT (c1) ON FOREIGN TABLE ft1 TO foreign_data_user; -- WARNING
+ GRANT UPDATE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT UPDATE (c1) ON FOREIGN TABLE ft1 TO foreign_data_user; -- WARNING
+ GRANT DELETE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT TRUNCATE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT REFERENCES ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT REFERENCES (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;-- WARNING
+ GRANT TRIGGER ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT USAGE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ GRANT EXECUTE ON FOREIGN TABLE ft1 TO foreign_data_user; -- ERROR
+ -- omitting FOREIGN TABLE
+ GRANT SELECT ON ft1 TO foreign_data_user;
+ GRANT SELECT (c1) ON ft1 TO foreign_data_user;
+ GRANT INSERT ON ft1 TO foreign_data_user; -- ERROR
+ GRANT INSERT (c1) ON ft1 TO foreign_data_user; -- WARNING
+ GRANT UPDATE ON ft1 TO foreign_data_user; -- ERROR
+ GRANT UPDATE (c1) ON ft1 TO foreign_data_user; -- WARNING
+ GRANT DELETE ON ft1 TO foreign_data_user; -- ERROR
+ GRANT TRUNCATE ON ft1 TO foreign_data_user; -- ERROR
+ GRANT REFERENCES ON ft1 TO foreign_data_user; -- ERROR
+ GRANT REFERENCES (c1) ON ft1 TO foreign_data_user ;-- WARNING
+ GRANT TRIGGER ON ft1 TO foreign_data_user; -- ERROR
+ GRANT USAGE ON ft1 TO foreign_data_user; -- ERROR
+ GRANT EXECUTE ON ft1 TO foreign_data_user; -- ERROR
+ \dp ft1
+ REVOKE SELECT ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE SELECT (c1) ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE INSERT ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE UPDATE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE DELETE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE TRUNCATE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE REFERENCES ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE TRIGGER ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE USAGE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ REVOKE EXECUTE ON FOREIGN TABLE ft1 FROM foreign_data_user; -- ERROR
+ \dp ft1
+ -- GRANT/REVOKE ALL
+ GRANT ALL ON FOREIGN TABLE ft1 TO foreign_data_user;
+ GRANT ALL (c1) ON FOREIGN TABLE ft1 TO foreign_data_user;
+ \dp ft1
+ REVOKE ALL ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ REVOKE ALL (c1) ON FOREIGN TABLE ft1 FROM foreign_data_user;
+ \dp ft1
+ -- GRANT ALL FOREIGN TABLES IN SCHEMA
+ ALTER FOREIGN TABLE ft1 SET SCHEMA foreign_schema;
+ CREATE FOREIGN TABLE foreign_schema.ft2 (c1 int) SERVER sc;
+ GRANT SELECT ON ALL FOREIGN TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ GRANT SELECT (c1) ON ALL FOREIGN TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ GRANT UPDATE ON ALL TABLES IN SCHEMA foreign_schema TO foreign_data_user;
+ \dp foreign_schema.ft*
+ REVOKE SELECT ON ALL FOREIGN TABLES IN SCHEMA foreign_schema FROM foreign_data_user;
+ REVOKE SELECT (c1) ON ALL FOREIGN TABLES IN SCHEMA foreign_schema FROM foreign_data_user;
+ \dp foreign_schema.ft*
+ ALTER FOREIGN TABLE foreign_schema.ft1 SET SCHEMA public;
+ DROP FOREIGN TABLE foreign_schema.ft2;
+
-- ALTER FOREIGN TABLE
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
Shigeru Hanada <hanada@metrosystems.co.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@sss.pgh.pa.us> wrote:
Shigeru Hanada <hanada@metrosystems.co.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@gmail.com> wrote:
On Wed, Apr 20, 2011 at 9:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shigeru Hanada <hanada@metrosystems.co.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@gmail.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@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.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@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.
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@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;
GRANTIf 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@gmail.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@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.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
Attachments:
foreign_table_privs.patchtext/plain; name=foreign_table_privs.patchDownload
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index 93e8332..689aba5 100644
*** a/doc/src/sgml/ref/grant.sgml
--- b/doc/src/sgml/ref/grant.sgml
*************** GRANT <replaceable class="PARAMETER">rol
*** 101,107 ****
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
only for tables, sequences, and functions (but note that <literal>ALL
! TABLES</> is considered to include views).
</para>
<para>
--- 101,107 ----
There is also an option to grant privileges on all objects of the same
type within one or more schemas. This functionality is currently supported
only for tables, sequences, and functions (but note that <literal>ALL
! TABLES</> is considered to include views and foreign tables).
</para>
<para>
diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index c623fb7..452a0ea 100644
*** a/src/backend/catalog/information_schema.sql
--- b/src/backend/catalog/information_schema.sql
*************** CREATE VIEW _pg_foreign_tables AS
*** 2557,2564 ****
WHERE w.oid = s.srvfdw
AND u.oid = c.relowner
AND (pg_has_role(c.relowner, 'USAGE')
! OR has_table_privilege(c.oid, 'SELECT')
! OR has_any_column_privilege(c.oid, 'SELECT'))
AND n.oid = c.relnamespace
AND c.oid = t.ftrelid
AND c.relkind = 'f'
--- 2557,2564 ----
WHERE w.oid = s.srvfdw
AND u.oid = c.relowner
AND (pg_has_role(c.relowner, 'USAGE')
! OR has_table_privilege(c.oid, 'SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER')
! OR has_any_column_privilege(c.oid, 'SELECT, INSERT, UPDATE, REFERENCES'))
AND n.oid = c.relnamespace
AND c.oid = t.ftrelid
AND c.relkind = 'f'
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 30366d2..e474a69 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpTable(Archive *fout, TableInfo *tbin
*** 11804,11810 ****
namecopy = strdup(fmtId(tbinfo->dobj.name));
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
(tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" :
- (tbinfo->relkind == RELKIND_FOREIGN_TABLE) ? "FOREIGN TABLE" :
"TABLE",
namecopy, NULL, tbinfo->dobj.name,
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
--- 11804,11809 ----
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 3ef2fa4..02f4aea 100644
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** psql_completion(char *text, int start, i
*** 2234,2240 ****
" UNION SELECT 'DATABASE'"
" UNION SELECT 'FOREIGN DATA WRAPPER'"
" UNION SELECT 'FOREIGN SERVER'"
- " UNION SELECT 'FOREIGN TABLE'"
" UNION SELECT 'FUNCTION'"
" UNION SELECT 'LANGUAGE'"
" UNION SELECT 'LARGE OBJECT'"
--- 2234,2239 ----
2011/5/11 Shigeru Hanada <hanada@metrosystems.co.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