Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/05/26 3:15, Tom Lane wrote: Etsuro Fujita writes: On 2015/05/22 23:50, Tom Lane wrote: So I think worrying about convalidated is pretty pointless. Really, it is up to the user to determine what constraints should be attached to the foreign table, and IMPORT FOREIGN SCHEMA can't help much. Agreed. So, I'd like to propose to update the docs as above. Please find attached a patch. The existing sentence "Checking other types of constraints is always left to the remote server." sounds to me that constraints on foreign tables are enforced locally when updating the foreign tables. So, I'd also like to propose to remove that sentence. Maybe I'm missing something though. Yeah, I agree this documentation is inadequate; but I think it'd be a good thing to spell out the reason why there's no support for importing check constraints. I committed the attached enlarged version. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
Etsuro Fujita writes: > On 2015/05/22 23:50, Tom Lane wrote: >> So I think worrying about convalidated is pretty pointless. Really, >> it is up to the user to determine what constraints should be attached >> to the foreign table, and IMPORT FOREIGN SCHEMA can't help much. > Agreed. So, I'd like to propose to update the docs as above. Please > find attached a patch. The existing sentence "Checking other types of > constraints is always left to the remote server." sounds to me that > constraints on foreign tables are enforced locally when updating the > foreign tables. So, I'd also like to propose to remove that sentence. > Maybe I'm missing something though. Yeah, I agree this documentation is inadequate; but I think it'd be a good thing to spell out the reason why there's no support for importing check constraints. I committed the attached enlarged version. regards, tom lane diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 1079140..14b12e3 100644 *** a/doc/src/sgml/postgres-fdw.sgml --- b/doc/src/sgml/postgres-fdw.sgml *** *** 309,316 using . This command creates foreign table definitions on the local server that match tables or views present on the remote server. If the remote tables to be imported ! have columns of user-defined data types, the local server must have types ! of the same names. --- 309,316 using . This command creates foreign table definitions on the local server that match tables or views present on the remote server. If the remote tables to be imported ! have columns of user-defined data types, the local server must have ! compatible types of the same names. *** *** 361,369 Note that constraints other than NOT NULL will never be ! imported from the remote tables, since PostgreSQL ! does not support any other type of constraint on a foreign table. ! Checking other types of constraints is always left to the remote server. --- 361,376 Note that constraints other than NOT NULL will never be ! imported from the remote tables. Although PostgreSQL ! does support CHECK constraints on foreign tables, there is no ! provision for importing them automatically, because of the risk that a ! constraint expression could evaluate differently on the local and remote ! servers. Any such inconsistency in the behavior of a CHECK ! constraint could lead to hard-to-detect errors in query optimization. ! So if you wish to import CHECK constraints, you must do so ! manually, and you should verify the semantics of each one carefully. ! For more detail about the treatment of CHECK constraints on ! foreign tables, see . -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/05/22 23:50, Tom Lane wrote: > Etsuro Fujita writes: >> I agree with you on that point. And I think one option for that is that >> IMPORT FOREIGN SCHEMA only imports CHECK constraints of remote tables >> from a remote server that have convalidated = true. (If doing so, we >> wouldn't need to allow IMPORT FOREIGN SCHEMA to return ALTER FOREIGN >> TABLE statements.) But I'm not sure that that is a good idea. ISTM it >> would be better for IMPORT FOREIGN SCHEMA just to imports all CHECK >> constraints of remote tables, reflecting convalidated, and that we leave >> it to the user to do VALIDATE CONSTRAINT for locally defined constraints >> that have convalidated = false when matching constraints are validated >> on the remote server. > > It would only be safe to automatically import CHECK constraints if they > contain no expressions that could evaluate differently on remote and local > --- IOW, only expressions that would be safe to transmit as remote query > conditions. I don't really think we should try to do that at all. > > For starters, while it might seem like we could use is_foreign_expr() > on the conditions, there's no guarantee that the local server accurately > knows what functions on the foreign server are really safe. The "is safe > to transmit" condition isn't symmetric. > > For that matter, there's no guarantee that we could even parse the > remote's constraint condition text without failing --- it might use SQL > features we don't have. (We have that risk already for DEFAULT > expressions, of course, but those tend to be a lot simpler.) I missed that point (because I was only thinking to use this in a sharding environment where all the servers have the same OS and PG). Thanks for pointing it out! > So I think worrying about convalidated is pretty pointless. Really, > it is up to the user to determine what constraints should be attached > to the foreign table, and IMPORT FOREIGN SCHEMA can't help much. Agreed. So, I'd like to propose to update the docs as above. Please find attached a patch. The existing sentence "Checking other types of constraints is always left to the remote server." sounds to me that constraints on foreign tables are enforced locally when updating the foreign tables. So, I'd also like to propose to remove that sentence. Maybe I'm missing something though. I'll change the name and category of this in CF 2015-06, accordingly. Best regards, Etsuro Fujita diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 1079140..8e39052 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -361,9 +361,11 @@ Note that constraints other than NOT NULL will never be -imported from the remote tables, since PostgreSQL -does not support any other type of constraint on a foreign table. -Checking other types of constraints is always left to the remote server. +imported from the remote tables. Other than NOT NULL, +PostgreSQL supports CHECK constraints +on foreign tables. However, it is up to you to determine which +CHECK constraints on the remote tables should be attached +to the foreign table definitions. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
Etsuro Fujita writes: > I agree with you on that point. And I think one option for that is that > IMPORT FOREIGN SCHEMA only imports CHECK constraints of remote tables > from a remote server that have convalidated = true. (If doing so, we > wouldn't need to allow IMPORT FOREIGN SCHEMA to return ALTER FOREIGN > TABLE statements.) But I'm not sure that that is a good idea. ISTM it > would be better for IMPORT FOREIGN SCHEMA just to imports all CHECK > constraints of remote tables, reflecting convalidated, and that we leave > it to the user to do VALIDATE CONSTRAINT for locally defined constraints > that have convalidated = false when matching constraints are validated > on the remote server. It would only be safe to automatically import CHECK constraints if they contain no expressions that could evaluate differently on remote and local --- IOW, only expressions that would be safe to transmit as remote query conditions. I don't really think we should try to do that at all. For starters, while it might seem like we could use is_foreign_expr() on the conditions, there's no guarantee that the local server accurately knows what functions on the foreign server are really safe. The "is safe to transmit" condition isn't symmetric. For that matter, there's no guarantee that we could even parse the remote's constraint condition text without failing --- it might use SQL features we don't have. (We have that risk already for DEFAULT expressions, of course, but those tend to be a lot simpler.) So I think worrying about convalidated is pretty pointless. Really, it is up to the user to determine what constraints should be attached to the foreign table, and IMPORT FOREIGN SCHEMA can't help much. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/05/22 1:36, Robert Haas wrote: On Mon, May 18, 2015 at 4:03 AM, Etsuro Fujita wrote: On 2015/05/16 3:32, Robert Haas wrote: On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita wrote: On second thought, I noticed that as for this option, we cannot live without allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements because we cannot declare the convalidated information in the CREATE FOREIGN TABLE statement. So, I think we shoould also allow it to return ALTER FOREIGN TABLE statements. Am I right? Isn't convalidated utterly meaningless for constraints on foreign tables? Let me explain. I think that convalidated would be *essential* for accurately performing relation_excluded_by_constraints for foreign tables like plain tables; if we didn't have that information, I think we would fail to accurately detect whether foreign tables need not be scanned. My point is that any constraint on a foreign table is just something we HOPE the remote side is enforcing. Regardless of whether convalidated is true or false locally, it could have some other value on the remote side, or the constraint might not exist on the remote side at all. I agree with you on that point. And I think one option for that is that IMPORT FOREIGN SCHEMA only imports CHECK constraints of remote tables from a remote server that have convalidated = true. (If doing so, we wouldn't need to allow IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements.) But I'm not sure that that is a good idea. ISTM it would be better for IMPORT FOREIGN SCHEMA just to imports all CHECK constraints of remote tables, reflecting convalidated, and that we leave it to the user to do VALIDATE CONSTRAINT for locally defined constraints that have convalidated = false when matching constraints are validated on the remote server. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On Mon, May 18, 2015 at 4:03 AM, Etsuro Fujita wrote: > On 2015/05/16 3:32, Robert Haas wrote: >> On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita >> wrote: >>> >>> On second thought, I noticed that as for this option, we cannot live >>> without >>> allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements >>> because we cannot declare the convalidated information in the CREATE >>> FOREIGN >>> TABLE statement. So, I think we shoould also allow it to return ALTER >>> FOREIGN TABLE statements. Am I right? >> >> Isn't convalidated utterly meaningless for constraints on foreign tables? > > Let me explain. I think that convalidated would be *essential* for > accurately performing relation_excluded_by_constraints for foreign tables > like plain tables; if we didn't have that information, I think we would fail > to accurately detect whether foreign tables need not be scanned. My point is that any constraint on a foreign table is just something we HOPE the remote side is enforcing. Regardless of whether convalidated is true or false locally, it could have some other value on the remote side, or the constraint might not exist on the remote side at all. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015-05-18 PM 06:45, Etsuro Fujita wrote: > On 2015/05/18 17:44, Amit Langote wrote: >> On 2015-05-18 PM 05:03, Etsuro Fujita wrote: >>> Let me explain. I think that convalidated would be *essential* for >>> accurately >>> performing relation_excluded_by_constraints for foreign tables like plain >>> tables; if we didn't have that information, I think we would fail to >>> accurately detect whether foreign tables need not be scanned. > >> So, if we decide to reflect remote/accurate convalidated locally by using the >> method you propose then would it mean only the foreign tables imported using >> IMPORT FOREIGN SCHEMA will have accurate convalidated info? AFAICS, there is >> no way to ensure the same for foreign tables created in normal way (CREATE >> FOREIGN TABLE) nor is there a way to propagate ALTER TABLE ... VALIDATE >> CONSTRAINT on a foreign table to remote side so that convalidated on the >> local >> side really matches the reality (on remote side). Does that cause >> inconsistent >> behavior or am I missing something? > > We now allow ALTER FOREIGN TABLE ADD CONSTRAINT NOT VALID. So after creating > foreign tables in normal way (CREATE FOREIGN TABLE), we can manually define > CHECK constraints that have convalidated = false by that command. > Ah, so creating a NOT VALID constraint *prevents* potentially wrong exclusion of a foreign table at least based on that constraint. Thanks for reminding of that option. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/05/18 17:44, Amit Langote wrote: On 2015-05-18 PM 05:03, Etsuro Fujita wrote: On 2015/05/16 3:32, Robert Haas wrote: On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita wrote: On second thought, I noticed that as for this option, we cannot live without allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements because we cannot declare the convalidated information in the CREATE FOREIGN TABLE statement. So, I think we shoould also allow it to return ALTER FOREIGN TABLE statements. Am I right? Isn't convalidated utterly meaningless for constraints on foreign tables? Let me explain. I think that convalidated would be *essential* for accurately performing relation_excluded_by_constraints for foreign tables like plain tables; if we didn't have that information, I think we would fail to accurately detect whether foreign tables need not be scanned. So, if we decide to reflect remote/accurate convalidated locally by using the method you propose then would it mean only the foreign tables imported using IMPORT FOREIGN SCHEMA will have accurate convalidated info? AFAICS, there is no way to ensure the same for foreign tables created in normal way (CREATE FOREIGN TABLE) nor is there a way to propagate ALTER TABLE ... VALIDATE CONSTRAINT on a foreign table to remote side so that convalidated on the local side really matches the reality (on remote side). Does that cause inconsistent behavior or am I missing something? We now allow ALTER FOREIGN TABLE ADD CONSTRAINT NOT VALID. So after creating foreign tables in normal way (CREATE FOREIGN TABLE), we can manually define CHECK constraints that have convalidated = false by that command. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015-05-18 PM 05:03, Etsuro Fujita wrote: > On 2015/05/16 3:32, Robert Haas wrote: >> On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita >> wrote: >>> On second thought, I noticed that as for this option, we cannot live without >>> allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements >>> because we cannot declare the convalidated information in the CREATE FOREIGN >>> TABLE statement. So, I think we shoould also allow it to return ALTER >>> FOREIGN TABLE statements. Am I right? >> >> Isn't convalidated utterly meaningless for constraints on foreign tables? > > Let me explain. I think that convalidated would be *essential* for accurately > performing relation_excluded_by_constraints for foreign tables like plain > tables; if we didn't have that information, I think we would fail to > accurately detect whether foreign tables need not be scanned. > So, if we decide to reflect remote/accurate convalidated locally by using the method you propose then would it mean only the foreign tables imported using IMPORT FOREIGN SCHEMA will have accurate convalidated info? AFAICS, there is no way to ensure the same for foreign tables created in normal way (CREATE FOREIGN TABLE) nor is there a way to propagate ALTER TABLE ... VALIDATE CONSTRAINT on a foreign table to remote side so that convalidated on the local side really matches the reality (on remote side). Does that cause inconsistent behavior or am I missing something? Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/05/16 3:32, Robert Haas wrote: On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita wrote: On second thought, I noticed that as for this option, we cannot live without allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements because we cannot declare the convalidated information in the CREATE FOREIGN TABLE statement. So, I think we shoould also allow it to return ALTER FOREIGN TABLE statements. Am I right? Isn't convalidated utterly meaningless for constraints on foreign tables? Let me explain. I think that convalidated would be *essential* for accurately performing relation_excluded_by_constraints for foreign tables like plain tables; if we didn't have that information, I think we would fail to accurately detect whether foreign tables need not be scanned. BTW, I don't know if it's a good idea to import connoinherit from the remote and then reflect that information on the local. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita wrote: > On second thought, I noticed that as for this option, we cannot live without > allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements > because we cannot declare the convalidated information in the CREATE FOREIGN > TABLE statement. So, I think we shoould also allow it to return ALTER > FOREIGN TABLE statements. Am I right? Isn't convalidated utterly meaningless for constraints on foreign tables? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/04/30 2:10, Robert Haas wrote: On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier wrote: Authorizing ALTER FOREIGN TABLE as query string that a FDW can use with IMPORT FOREIGN SCHEMA is a different feature than what is proposed in this patch, aka an option for postgres_fdw and meritates a discussion on its own because it impacts all the FDWs and not only postgres_fdw. Now, related to this patch, we could live without authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does authorize the definition of CHECK constraints. I agree. I don't think there's a huge problem with allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements, but it doesn't really seem to be necessary. I don't see why we can't just declare the CHECK constraints in the CREATE FOREIGN TABLE statement instead of adding more DDL. On second thought, I noticed that as for this option, we cannot live without allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements because we cannot declare the convalidated information in the CREATE FOREIGN TABLE statement. So, I think we shoould also allow it to return ALTER FOREIGN TABLE statements. Am I right? Comments welcome. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/04/30 17:15, Etsuro Fujita wrote: On 2015/04/30 2:10, Robert Haas wrote: On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier wrote: Authorizing ALTER FOREIGN TABLE as query string that a FDW can use with IMPORT FOREIGN SCHEMA is a different feature than what is proposed in this patch, aka an option for postgres_fdw and meritates a discussion on its own because it impacts all the FDWs and not only postgres_fdw. Now, related to this patch, we could live without authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does authorize the definition of CHECK constraints. I agree. I don't think there's a huge problem with allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements, but it doesn't really seem to be necessary. I don't see why we can't just declare the CHECK constraints in the CREATE FOREIGN TABLE statement instead of adding more DDL. I think that it'd improve the convenience of an FDW developer writing ImportForeignSchema() to allow it to return ALTER FOREIGN TABLE (and perhaps DROP FOREIGN TABLE) as well, but I agree that that needs another discussion. So I'll leave that as is and update the patch as discussed above. Done. Please find attached an updated version of the patch. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 3415,3420 CREATE TABLE import_source.t2 (c1 int default 42, c2 varchar NULL, c3 text colla --- 3415,3421 CREATE TYPE typ1 AS (m1 int, m2 varchar); CREATE TABLE import_source.t3 (c1 timestamptz default now(), c2 typ1); CREATE TABLE import_source."x 4" (c1 float8, "C 2" text, c3 varchar(42)); + ALTER TABLE import_source."x 4" ADD CHECK (c1 >= 0); CREATE TABLE import_source."x 5" (c1 float8); ALTER TABLE import_source."x 5" DROP COLUMN c1; CREATE SCHEMA import_dest1; *** *** 3462,3467 FDW Options: (schema_name 'import_source', table_name 't3') --- 3463,3470 c1 | double precision | | (column_name 'c1') C 2| text | | (column_name 'C 2') c3 | character varying(42) | | (column_name 'c3') + Check constraints: + "x 4_c1_check" CHECK (c1 >= 0::double precision) Server: loopback FDW Options: (schema_name 'import_source', table_name 'x 4') *** *** 3518,3523 FDW Options: (schema_name 'import_source', table_name 't3') --- 3521,3528 c1 | double precision | | (column_name 'c1') C 2| text | | (column_name 'C 2') c3 | character varying(42) | | (column_name 'c3') + Check constraints: + "x 4_c1_check" CHECK (c1 >= 0::double precision) Server: loopback FDW Options: (schema_name 'import_source', table_name 'x 4') *** *** 3529,3535 FDW Options: (schema_name 'import_source', table_name 'x 5') CREATE SCHEMA import_dest3; IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest3 ! OPTIONS (import_collate 'false', import_not_null 'false'); \det+ import_dest3 List of foreign tables Schema| Table | Server | FDW Options | Description --- 3534,3540 CREATE SCHEMA import_dest3; IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest3 ! OPTIONS (import_collate 'false', import_not_null 'false', import_check 'false'); \det+ import_dest3 List of foreign tables Schema| Table | Server | FDW Options | Description *** a/contrib/postgres_fdw/postgres_fdw.c --- b/contrib/postgres_fdw/postgres_fdw.c *** *** 2584,2594 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2584,2597 bool import_collate = true; bool import_default = false; bool import_not_null = true; + bool import_check = true; ForeignServer *server; UserMapping *mapping; PGconn *conn; StringInfoData buf; + StringInfoData buf2; PGresult *volatile res = NULL; + PGresult *volatile res2 = NULL; int numrows, i; ListCell *lc; *** *** 2604,2609 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2607,2614 import_default = defGetBoolean(def); else if (strcmp(def->defname, "import_not_null") == 0) import_not_null = defGetBoolean(def); + else if (strcmp(def->defname, "import_check") == 0) + import_check = defGetBoolean(def); else ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), *** *** 2624,2629 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2629,2635 /* Create workspace for strings */ initStringInfo(&buf); + initStringInfo(&buf2); /* In what follows, do not risk leaking any PGresults. */
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/04/30 2:10, Robert Haas wrote: On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier wrote: Authorizing ALTER FOREIGN TABLE as query string that a FDW can use with IMPORT FOREIGN SCHEMA is a different feature than what is proposed in this patch, aka an option for postgres_fdw and meritates a discussion on its own because it impacts all the FDWs and not only postgres_fdw. Now, related to this patch, we could live without authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does authorize the definition of CHECK constraints. I agree. I don't think there's a huge problem with allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements, but it doesn't really seem to be necessary. I don't see why we can't just declare the CHECK constraints in the CREATE FOREIGN TABLE statement instead of adding more DDL. I think that it'd improve the convenience of an FDW developer writing ImportForeignSchema() to allow it to return ALTER FOREIGN TABLE (and perhaps DROP FOREIGN TABLE) as well, but I agree that that needs another discussion. So I'll leave that as is and update the patch as discussed above. Thanks for the comments! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier wrote: > Authorizing ALTER FOREIGN TABLE as query string that a FDW can use > with IMPORT FOREIGN SCHEMA is a different feature than what is > proposed in this patch, aka an option for postgres_fdw and meritates a > discussion on its own because it impacts all the FDWs and not only > postgres_fdw. Now, related to this patch, we could live without > authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does > authorize the definition of CHECK constraints. I agree. I don't think there's a huge problem with allowing IMPORT FOREIGN SCHEMA to return ALTER FOREIGN TABLE statements, but it doesn't really seem to be necessary. I don't see why we can't just declare the CHECK constraints in the CREATE FOREIGN TABLE statement instead of adding more DDL. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On Mon, Apr 27, 2015 at 4:20 PM, Etsuro Fujita wrote: > On 2015/04/27 15:51, Michael Paquier wrote: >> >> On Mon, Apr 27, 2015 at 3:15 PM, Etsuro Fujita >> wrote: >>> >>> I noticed that there is no postgres_fdw option to control whether check >>> constraints on remote tables are included in the definitions of foreign >>> tables imported from a remote PG server when performing IMPORT FOREIGN >>> SCHEMA, while we now allow check constraints on foreign tables. > > >> I guess that the addition of this option makes sense, but I think that >> this patch is doing it wrong by using ALTER FOREIGN TABLE and by >> changing the queries authorized in ImportForeignSchema(). The point of >> IMPORT FOREIGN SCHEMA is to authorize only CREATE FOREIGN TABLE and >> not other types of queries, not to mention that CREATE FOREIGN TABLE >> supports the definitions of constraints at table and column-level. > > > I don't think so. IMO, I think it'd be more convenient and flexible to > allow ALTER FOREIGN TABLE for creating foreign table definitions during > ImportForeignSchema(). Authorizing ALTER FOREIGN TABLE as query string that a FDW can use with IMPORT FOREIGN SCHEMA is a different feature than what is proposed in this patch, aka an option for postgres_fdw and meritates a discussion on its own because it impacts all the FDWs and not only postgres_fdw. Now, related to this patch, we could live without authorizing ALTER FOREIGN TABLE because CREATE FOREIGN TABLE does authorize the definition of CHECK constraints. Also, I imagine that it would be more natural to combine the fetch of the constraint expression in the existing query with a join on conrelid so as to not replicate the logic that your patch is doing with FDW_IMPORT_SCHEMA_LIMIT_TO and FDW_IMPORT_SCHEMA_EXCEPT that reuses the same logic to re-build the [ NOT ] IN clause. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On 2015/04/27 15:51, Michael Paquier wrote: On Mon, Apr 27, 2015 at 3:15 PM, Etsuro Fujita wrote: I noticed that there is no postgres_fdw option to control whether check constraints on remote tables are included in the definitions of foreign tables imported from a remote PG server when performing IMPORT FOREIGN SCHEMA, while we now allow check constraints on foreign tables. I guess that the addition of this option makes sense, but I think that this patch is doing it wrong by using ALTER FOREIGN TABLE and by changing the queries authorized in ImportForeignSchema(). The point of IMPORT FOREIGN SCHEMA is to authorize only CREATE FOREIGN TABLE and not other types of queries, not to mention that CREATE FOREIGN TABLE supports the definitions of constraints at table and column-level. I don't think so. IMO, I think it'd be more convenient and flexible to allow ALTER FOREIGN TABLE for creating foreign table definitions during ImportForeignSchema(). Thanks for the comment! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing importing option of postgres_fdw
On Mon, Apr 27, 2015 at 3:15 PM, Etsuro Fujita wrote: > Hi, > > I noticed that there is no postgres_fdw option to control whether check > constraints on remote tables are included in the definitions of foreign > tables imported from a remote PG server when performing IMPORT FOREIGN > SCHEMA, while we now allow check constraints on foreign tables. > > Attached is a patch for that. I'll add this to the next CF. I guess that the addition of this option makes sense, but I think that this patch is doing it wrong by using ALTER FOREIGN TABLE and by changing the queries authorized in ImportForeignSchema(). The point of IMPORT FOREIGN SCHEMA is to authorize only CREATE FOREIGN TABLE and not other types of queries, not to mention that CREATE FOREIGN TABLE supports the definitions of constraints at table and column-level. Logically, this patch should just create diffs with postgres_fdw and nothing else. Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Missing importing option of postgres_fdw
Hi, I noticed that there is no postgres_fdw option to control whether check constraints on remote tables are included in the definitions of foreign tables imported from a remote PG server when performing IMPORT FOREIGN SCHEMA, while we now allow check constraints on foreign tables. Attached is a patch for that. I'll add this to the next CF. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 3415,3420 CREATE TABLE import_source.t2 (c1 int default 42, c2 varchar NULL, c3 text colla --- 3415,3421 CREATE TYPE typ1 AS (m1 int, m2 varchar); CREATE TABLE import_source.t3 (c1 timestamptz default now(), c2 typ1); CREATE TABLE import_source."x 4" (c1 float8, "C 2" text, c3 varchar(42)); + ALTER TABLE import_source."x 4" ADD CONSTRAINT c1positive CHECK (c1 >= 0); CREATE TABLE import_source."x 5" (c1 float8); ALTER TABLE import_source."x 5" DROP COLUMN c1; CREATE SCHEMA import_dest1; *** *** 3462,3467 FDW Options: (schema_name 'import_source', table_name 't3') --- 3463,3470 c1 | double precision | | (column_name 'c1') C 2| text | | (column_name 'C 2') c3 | character varying(42) | | (column_name 'c3') + Check constraints: + "c1positive" CHECK (c1 >= 0::double precision) Server: loopback FDW Options: (schema_name 'import_source', table_name 'x 4') *** *** 3518,3523 FDW Options: (schema_name 'import_source', table_name 't3') --- 3521,3528 c1 | double precision | | (column_name 'c1') C 2| text | | (column_name 'C 2') c3 | character varying(42) | | (column_name 'c3') + Check constraints: + "c1positive" CHECK (c1 >= 0::double precision) Server: loopback FDW Options: (schema_name 'import_source', table_name 'x 4') *** *** 3529,3535 FDW Options: (schema_name 'import_source', table_name 'x 5') CREATE SCHEMA import_dest3; IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest3 ! OPTIONS (import_collate 'false', import_not_null 'false'); \det+ import_dest3 List of foreign tables Schema| Table | Server | FDW Options | Description --- 3534,3540 CREATE SCHEMA import_dest3; IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest3 ! OPTIONS (import_collate 'false', import_not_null 'false', import_check 'false'); \det+ import_dest3 List of foreign tables Schema| Table | Server | FDW Options | Description *** a/contrib/postgres_fdw/postgres_fdw.c --- b/contrib/postgres_fdw/postgres_fdw.c *** *** 2584,2589 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2584,2590 bool import_collate = true; bool import_default = false; bool import_not_null = true; + bool import_check = true; ForeignServer *server; UserMapping *mapping; PGconn *conn; *** *** 2604,2609 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2605,2612 import_default = defGetBoolean(def); else if (strcmp(def->defname, "import_not_null") == 0) import_not_null = defGetBoolean(def); + else if (strcmp(def->defname, "import_check") == 0) + import_check = defGetBoolean(def); else ereport(ERROR, (errcode(ERRCODE_FDW_INVALID_OPTION_NAME), *** *** 2824,2829 postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid) --- 2827,2929 /* Clean up */ PQclear(res); res = NULL; + + /* + * Fetch all table (and column) check constraint data from this schema, + * possibly restricted by EXCEPT or LIMIT TO. + */ + if (import_check) + { + resetStringInfo(&buf); + + appendStringInfoString(&buf, + "SELECT relname, " + " conname, " + " pg_get_constraintdef(r.oid) " + "FROM pg_class c " + " JOIN pg_namespace n ON " + "relnamespace = n.oid " + " JOIN pg_constraint r ON " + "conrelid = c.oid AND contype = 'c' "); + + appendStringInfoString(&buf, + "WHERE c.relkind IN ('r', 'f') " + " AND n.nspname = "); + deparseStringLiteral(&buf, stmt->remote_schema); + + /* Apply restrictions for LIMIT TO and EXCEPT */ + if (stmt->list_type == FDW_IMPORT_SCHEMA_LIMIT_TO || + stmt->list_type == FDW_IMPORT_SCHEMA_EXCEPT) + { + bool first_item = true; + + appendStringInfoString(&buf, " AND c.relname "); + if (stmt->list_type == FDW_IMPORT_SCHEMA_EXCEPT) + appendStringInfoString(&buf, "NOT "); + appendStringInfoString(&buf, "IN ("); + + /* A