Missing importing option of postgres_fdw

Started by Etsuro Fujitaover 10 years ago19 messages
#1Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
1 attachment(s)

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

Attachments:

postgres_fdw-import_check-v1.patchtext/x-diff; name=postgres_fdw-import_check-v1.patchDownload
*** 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 (");
+ 
+ 				/* Append list of table names within IN clause */
+ 				foreach(lc, stmt->table_list)
+ 				{
+ 					RangeVar   *rv = (RangeVar *) lfirst(lc);
+ 
+ 					if (first_item)
+ 						first_item = false;
+ 					else
+ 						appendStringInfoString(&buf, ", ");
+ 					deparseStringLiteral(&buf, rv->relname);
+ 				}
+ 				appendStringInfoString(&buf, ")");
+ 			}
+ 
+ 			/* Append ORDER BY at the end of query to ensure output ordering */
+ 			appendStringInfo(&buf, " ORDER BY c.relname, r.conname");
+ 
+ 			/* Fetch the data */
+ 			res = PQexec(conn, buf.data);
+ 			if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ 				pgfdw_report_error(ERROR, res, conn, false, buf.data);
+ 
+ 			/* Process results */
+ 			numrows = PQntuples(res);
+ 			/* note: incrementation of i happens in inner loop's while() test */
+ 			for (i = 0; i < numrows;)
+ 			{
+ 				char	   *tablename = PQgetvalue(res, i, 0);
+ 				bool		first_item = true;
+ 
+ 				resetStringInfo(&buf);
+ 				appendStringInfo(&buf, "ALTER FOREIGN TABLE %s ",
+ 								 quote_identifier(tablename));
+ 
+ 				/* Scan all rows for this table */
+ 				do
+ 				{
+ 					char	   *conname = PQgetvalue(res, i, 1);
+ 					char	   *condef = PQgetvalue(res, i, 2);
+ 
+ 					if (first_item)
+ 						first_item = false;
+ 					else
+ 						appendStringInfoString(&buf, ", ");
+ 
+ 					/* Print constraint name and definition */
+ 					appendStringInfo(&buf, "ADD CONSTRAINT %s %s",
+ 									 quote_identifier(conname),
+ 									 condef);
+ 				}
+ 				while (++i < numrows &&
+ 					   strcmp(PQgetvalue(res, i, 0), tablename) == 0);
+ 
+ 				appendStringInfoString(&buf, ";");
+ 
+ 				commands = lappend(commands, pstrdup(buf.data));
+ 			}
+ 
+ 			/* Clean up */
+ 			PQclear(res);
+ 			res = NULL;
+ 		}
  	}
  	PG_CATCH();
  	{
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 785,790 **** CREATE TABLE import_source.t2 (c1 int default 42, c2 varchar NULL, c3 text colla
--- 785,791 ----
  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;
  
***************
*** 801,807 **** IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest2
  \d import_dest2.*
  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
  \d import_dest3.*
  
--- 802,808 ----
  \d import_dest2.*
  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
  \d import_dest3.*
  
*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***************
*** 709,715 **** ImportForeignSchema (ImportForeignSchemaStmt *stmt, Oid serverOid);
       called when executing <xref linkend="sql-importforeignschema">, and is
       passed the parse tree for that statement, as well as the OID of the
       foreign server to use.  It should return a list of C strings, each of
!      which must contain a <xref linkend="sql-createforeigntable"> command.
       These strings will be parsed and executed by the core server.
      </para>
  
--- 709,716 ----
       called when executing <xref linkend="sql-importforeignschema">, and is
       passed the parse tree for that statement, as well as the OID of the
       foreign server to use.  It should return a list of C strings, each of
!      which must contain a <xref linkend="sql-createforeigntable"> or
!      <xref linkend="sql-alterforeigntable"> command.
       These strings will be parsed and executed by the core server.
      </para>
  
***************
*** 736,742 **** ImportForeignSchema (ImportForeignSchemaStmt *stmt, Oid serverOid);
       The FDW may ignore the <structfield>local_schema</> field of
       the <structname>ImportForeignSchemaStmt</>, because the core server
       will automatically insert that name into the parsed <command>CREATE
!      FOREIGN TABLE</> commands.
      </para>
  
      <para>
--- 737,743 ----
       The FDW may ignore the <structfield>local_schema</> field of
       the <structname>ImportForeignSchemaStmt</>, because the core server
       will automatically insert that name into the parsed <command>CREATE
!      FOREIGN TABLE</> and <command>ALTER FOREIGN TABLE</> commands.
      </para>
  
      <para>
*** a/doc/src/sgml/postgres-fdw.sgml
--- b/doc/src/sgml/postgres-fdw.sgml
***************
*** 349,360 ****
        </para>
       </listitem>
      </varlistentry>
     </variablelist>
  
     <para>
!     Note that constraints other than <literal>NOT NULL</> will never be
!     imported from the remote tables, since <productname>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.
     </para>
    </sect3>
--- 349,371 ----
        </para>
       </listitem>
      </varlistentry>
+     <varlistentry>
+      <term><literal>import_check</literal></term>
+      <listitem>
+       <para>
+        This option controls whether column and table <literal>CHECK</>
+        constraints are included in the definitions of foreign tables imported
+        from a foreign server. The default is <literal>true</>.
+       </para>
+      </listitem>
+     </varlistentry>
     </variablelist>
  
     <para>
!     Note that constraints other than <literal>NOT NULL</> and
!     <literal>CHECK</> will never be imported from the remote tables, since
!     <productname>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.
     </para>
    </sect3>
*** a/src/backend/commands/foreigncmds.c
--- b/src/backend/commands/foreigncmds.c
***************
*** 1571,1599 **** ImportForeignSchema(ImportForeignSchemaStmt *stmt)
  		 */
  		foreach(lc2, raw_parsetree_list)
  		{
! 			CreateForeignTableStmt *cstmt = lfirst(lc2);
  
  			/*
! 			 * Because we only allow CreateForeignTableStmt, we can skip parse
! 			 * analysis, rewrite, and planning steps here.
  			 */
! 			if (!IsA(cstmt, CreateForeignTableStmt))
  				elog(ERROR,
  					 "foreign-data wrapper \"%s\" returned incorrect statement type %d",
! 					 fdw->fdwname, (int) nodeTag(cstmt));
  
  			/* Ignore commands for tables excluded by filter options */
! 			if (!IsImportableForeignTable(cstmt->base.relation->relname, stmt))
  				continue;
  
  			/* Enable reporting of current table's name on error */
! 			callback_arg.tablename = cstmt->base.relation->relname;
  
  			/* Ensure creation schema is the one given in IMPORT statement */
! 			cstmt->base.relation->schemaname = pstrdup(stmt->local_schema);
  
  			/* Execute statement */
! 			ProcessUtility((Node *) cstmt,
  						   cmd,
  						   PROCESS_UTILITY_SUBCOMMAND, NULL,
  						   None_Receiver, NULL);
--- 1571,1608 ----
  		 */
  		foreach(lc2, raw_parsetree_list)
  		{
! 			Node	   *parsetree = lfirst(lc2);
! 			RangeVar   *rv;
  
  			/*
! 			 * Because we only allow CreateForeignTableStmt and AlterTableStmt,
! 			 * we can skip parse analysis, rewrite, and planning steps here.
  			 */
! 			if (!IsA(parsetree, CreateForeignTableStmt) &&
! 				!(IsA(parsetree, AlterTableStmt) &&
! 				  ((AlterTableStmt *) parsetree)->relkind == OBJECT_FOREIGN_TABLE))
  				elog(ERROR,
  					 "foreign-data wrapper \"%s\" returned incorrect statement type %d",
! 					 fdw->fdwname, (int) nodeTag(parsetree));
! 
! 			/* Get RangeVar */
! 			if (!IsA(parsetree, CreateForeignTableStmt))
! 				rv = ((AlterTableStmt *) parsetree)->relation;
! 			else
! 				rv = ((CreateForeignTableStmt *) parsetree)->base.relation;
  
  			/* Ignore commands for tables excluded by filter options */
! 			if (!IsImportableForeignTable(rv->relname, stmt))
  				continue;
  
  			/* Enable reporting of current table's name on error */
! 			callback_arg.tablename = rv->relname;
  
  			/* Ensure creation schema is the one given in IMPORT statement */
! 			rv->schemaname = pstrdup(stmt->local_schema);
  
  			/* Execute statement */
! 			ProcessUtility(parsetree,
  						   cmd,
  						   PROCESS_UTILITY_SUBCOMMAND, NULL,
  						   None_Receiver, NULL);
#2Michael Paquier
michael.paquier@gmail.com
In reply to: Etsuro Fujita (#1)
Re: Missing importing option of postgres_fdw

On Mon, Apr 27, 2015 at 3:15 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#3Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Michael Paquier (#2)
Re: 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
<fujita.etsuro@lab.ntt.co.jp> 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

#4Michael Paquier
michael.paquier@gmail.com
In reply to: Etsuro Fujita (#3)
Re: Missing importing option of postgres_fdw

On Mon, Apr 27, 2015 at 4:20 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2015/04/27 15:51, Michael Paquier wrote:

On Mon, Apr 27, 2015 at 3:15 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#5Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#4)
Re: Missing importing option of postgres_fdw

On Mon, Apr 27, 2015 at 7:47 AM, Michael Paquier
<michael.paquier@gmail.com> 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

#6Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#5)
Re: 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
<michael.paquier@gmail.com> 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

#7Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Etsuro Fujita (#6)
1 attachment(s)
Re: 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
<michael.paquier@gmail.com> 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

Attachments:

postgres_fdw-import_check-v2.patchtext/x-diff; name=postgres_fdw-import_check-v2.patchDownload
*** 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. */
  	PG_TRY();
***************
*** 2663,2669 **** postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
  								   "  attnotnull, "
  								   "  pg_get_expr(adbin, adrelid), "
  								   "  collname, "
! 								   "  collnsp.nspname "
  								   "FROM pg_class c "
  								   "  JOIN pg_namespace n ON "
  								   "    relnamespace = n.oid "
--- 2669,2677 ----
  								   "  attnotnull, "
  								   "  pg_get_expr(adbin, adrelid), "
  								   "  collname, "
! 								   "  collnsp.nspname, "
! 								   "  c.oid, "
! 								   "  relchecks "
  								   "FROM pg_class c "
  								   "  JOIN pg_namespace n ON "
  								   "    relnamespace = n.oid "
***************
*** 2683,2689 **** postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
  								   "  format_type(atttypid, atttypmod), "
  								   "  attnotnull, "
  								   "  pg_get_expr(adbin, adrelid), "
! 								   "  NULL, NULL "
  								   "FROM pg_class c "
  								   "  JOIN pg_namespace n ON "
  								   "    relnamespace = n.oid "
--- 2691,2699 ----
  								   "  format_type(atttypid, atttypmod), "
  								   "  attnotnull, "
  								   "  pg_get_expr(adbin, adrelid), "
! 								   "  NULL, NULL, "
! 								   "  c.oid, "
! 								   "  relchecks "
  								   "FROM pg_class c "
  								   "  JOIN pg_namespace n ON "
  								   "    relnamespace = n.oid "
***************
*** 2803,2808 **** postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
--- 2813,2860 ----
  			while (++i < numrows &&
  				   strcmp(PQgetvalue(res, i, 0), tablename) == 0);
  
+ 			/* Add all CHECK constraints if needed */
+ 			if (import_check)
+ 			{
+ 				char	   *tableoid = PQgetvalue(res, i - 1, 7);
+ 				bool		hascheck = (atoi(PQgetvalue(res, i - 1, 8)) > 0);
+ 
+ 				if (hascheck)
+ 				{
+ 					int			numchecks;
+ 					int			row;
+ 
+ 					/* Fetch all CHECK constraint data */
+ 					resetStringInfo(&buf2);
+ 					appendStringInfo(&buf2,
+ 									 "SELECT conname, "
+ 									 "  pg_get_constraintdef(r.oid) "
+ 									 "FROM pg_constraint r "
+ 									 "WHERE conrelid = '%s' AND contype = 'c' "
+ 									 "ORDER BY conname",
+ 									 tableoid);
+ 
+ 					/* Fetch the data */
+ 					res2 = PQexec(conn, buf2.data);
+ 					if (PQresultStatus(res2) != PGRES_TUPLES_OK)
+ 						pgfdw_report_error(ERROR, res2, conn, false, buf2.data);
+ 
+ 					/* Process results */
+ 					numchecks = PQntuples(res2);
+ 					for (row = 0; row < numchecks; row++)
+ 					{
+ 						char	   *condef = PQgetvalue(res2, row, 1);
+ 
+ 						appendStringInfoString(&buf, ",\n");
+ 						appendStringInfo(&buf, "  %s", condef);
+ 					}
+ 
+ 					/* Clean up */
+ 					PQclear(res2);
+ 					res2 = NULL;
+ 				}
+ 			}
+ 
  			/*
  			 * Add server name and table-level options.  We specify remote
  			 * schema and table name as options (the latter to ensure that
***************
*** 2829,2834 **** postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
--- 2881,2888 ----
  	{
  		if (res)
  			PQclear(res);
+ 		if (res2)
+ 			PQclear(res2);
  		PG_RE_THROW();
  	}
  	PG_END_TRY();
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 785,790 **** CREATE TABLE import_source.t2 (c1 int default 42, c2 varchar NULL, c3 text colla
--- 785,791 ----
  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;
  
***************
*** 801,807 **** IMPORT FOREIGN SCHEMA import_source FROM SERVER loopback INTO import_dest2
  \d import_dest2.*
  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
  \d import_dest3.*
  
--- 802,808 ----
  \d import_dest2.*
  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
  \d import_dest3.*
  
*** a/doc/src/sgml/postgres-fdw.sgml
--- b/doc/src/sgml/postgres-fdw.sgml
***************
*** 349,360 ****
        </para>
       </listitem>
      </varlistentry>
     </variablelist>
  
     <para>
!     Note that constraints other than <literal>NOT NULL</> will never be
!     imported from the remote tables, since <productname>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.
     </para>
    </sect3>
--- 349,371 ----
        </para>
       </listitem>
      </varlistentry>
+     <varlistentry>
+      <term><literal>import_check</literal></term>
+      <listitem>
+       <para>
+        This option controls whether column and table <literal>CHECK</>
+        constraints are included in the definitions of foreign tables imported
+        from a foreign server. The default is <literal>true</>.
+       </para>
+      </listitem>
+     </varlistentry>
     </variablelist>
  
     <para>
!     Note that constraints other than <literal>NOT NULL</> and
!     <literal>CHECK</> will never be imported from the remote tables, since
!     <productname>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.
     </para>
    </sect3>
#8Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Etsuro Fujita (#7)
Re: 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
<michael.paquier@gmail.com> 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

#9Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#8)
Re: Missing importing option of postgres_fdw

On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#10Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#9)
Re: 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
<fujita.etsuro@lab.ntt.co.jp> 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

#11Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Etsuro Fujita (#10)
Re: 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
<fujita.etsuro@lab.ntt.co.jp> 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

#12Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Amit Langote (#11)
Re: 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
<fujita.etsuro@lab.ntt.co.jp> 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

#13Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Etsuro Fujita (#12)
Re: 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

#14Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#10)
Re: Missing importing option of postgres_fdw

On Mon, May 18, 2015 at 4:03 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2015/05/16 3:32, Robert Haas wrote:

On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#15Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Robert Haas (#14)
Re: 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
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2015/05/16 3:32, Robert Haas wrote:

On Thu, May 14, 2015 at 6:37 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#16Tom Lane
tgl@sss.pgh.pa.us
In reply to: Etsuro Fujita (#15)
Re: Missing importing option of postgres_fdw

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

#17Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Tom Lane (#16)
1 attachment(s)
Re: Missing importing option of postgres_fdw

On 2015/05/22 23:50, Tom Lane wrote:

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

Attachments:

doc-postgres-fdw.patchtext/x-diff; name=doc-postgres-fdw.patchDownload
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 @@
 
    <para>
     Note that constraints other than <literal>NOT NULL</> will never be
-    imported from the remote tables, since <productname>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 <literal>NOT NULL</>,
+    <productname>PostgreSQL</> supports <literal>CHECK</> constraints
+    on foreign tables.  However, it is up to you to determine which
+    <literal>CHECK</> constraints on the remote tables should be attached
+    to the foreign table definitions.
    </para>
   </sect3>
  </sect2>
#18Tom Lane
tgl@sss.pgh.pa.us
In reply to: Etsuro Fujita (#17)
1 attachment(s)
Re: Missing importing option of postgres_fdw

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

Attachments:

postgres_fdw_check_constraint_doc.patchtext/x-diff; charset=us-ascii; name=postgres_fdw_check_constraint_doc.patchDownload
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 <xref linkend="sql-importforeignschema">.  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.
     </para>
  
     <para>
--- 309,316 ----
      using <xref linkend="sql-importforeignschema">.  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.
     </para>
  
     <para>
***************
*** 361,369 ****
  
     <para>
      Note that constraints other than <literal>NOT NULL</> will never be
!     imported from the remote tables, since <productname>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.
     </para>
    </sect3>
   </sect2>
--- 361,376 ----
  
     <para>
      Note that constraints other than <literal>NOT NULL</> will never be
!     imported from the remote tables.  Although <productname>PostgreSQL</>
!     does support <literal>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 <literal>CHECK</>
!     constraint could lead to hard-to-detect errors in query optimization.
!     So if you wish to import <literal>CHECK</> constraints, you must do so
!     manually, and you should verify the semantics of each one carefully.
!     For more detail about the treatment of <literal>CHECK</> constraints on
!     foreign tables, see <xref linkend="sql-createforeigntable">.
     </para>
    </sect3>
   </sect2>
#19Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Tom Lane (#18)
Re: Missing importing option of postgres_fdw

On 2015/05/26 3:15, Tom Lane wrote:

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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