Thread: 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 stuffForeign 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: 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@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: 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
Attachment
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 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
Attachment
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 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@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
Attachment
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; >> GRANT > >> If that works for a view, it also ought to work for a foreign table, >> which I think is what you were saying. > > Yeah, the existing precedent (not only for GRANT but for some other > things like ALTER TABLE) is that a command that says "TABLE" is allowed > to apply to other relation types if it makes sense to apply it. It's > only when you name some other object type that we get picky about the > relkind matching exactly. This is probably more historical than > anything else, but it's the precedent and we shouldn't make foreign > tables be the only thing not following the precedent. > >> So now I think this is just a documentation bug. > > If the code already works like that for foreign tables, then no > behavioral change is needed. OK, let's test that: rhaas=# create foreign data wrapper dummy; CREATE FOREIGN DATA WRAPPER rhaas=# create server s1 foreign data wrapper dummy; CREATE SERVER rhaas=# create foreign table ft (a int) server s1; CREATE FOREIGN TABLE rhaas=# grant delete on ft to bob; ERROR: foreign table "ft" only supports SELECT privileges rhaas=# grant delete on table ft to bob; ERROR: foreign table "ft" only supports SELECT privileges So, nope, not the same. Also for comparison: rhaas=# create sequence blarg; CREATE SEQUENCE rhaas=# grant delete on blarg to bob; WARNING: sequence "blarg" only supports USAGE, SELECT, and UPDATE privileges GRANT This appears to be because ExecGrant_Relation() has this: else if (pg_class_tuple->relkind == RELKIND_FOREIGN_TABLE) { if (this_privileges & ~((AclMode)ACL_ALL_RIGHTS_FOREIGN_TABLE)) { ereport(ERROR, (errcode(ERRCODE_INVALID_GRANT_OPERATION), errmsg("foreign table \"%s\" only supports SELECT privileges", NameStr(pg_class_tuple->relname)))); } } There's a similar stanza for sequences, but that one uses ereport(WARNING...) rather than ereport(ERROR...). We could either remove that stanza entirely (making foreign tables consistent with views) or change ERROR to WARNING (making it consistent with sequences). If we remove it entirely, then we'll presumably also want to remove this chunk further down: else if (pg_class_tuple->relkind == RELKIND_FOREIGN_TABLE && this_privileges & ~((AclMode)ACL_SELECT)) { /* Foreign tables have the same restriction as sequences. */ ereport(WARNING, (errcode(ERRCODE_INVALID_GRANT_OPERATION), errmsg("foreigntable \"%s\" only supports SELECT column privileges", NameStr(pg_class_tuple->relname)))); this_privileges&= (AclMode) ACL_SELECT; } Thoughts? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
Attachment
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