On Mon, Feb 17, 2014 at 6:28 PM, Andres Freund <and...@2ndquadrant.com> wrote:
> I don't think this really has gone above Needs Review yet.
I am not sure that this remark makes the review of this patch much
progressing :(
By the way, I spent some time looking at it and here are some comments:
- Regression tests added are too sensitive with the other tests. For
example by not dropping tables or creating new tables on another tests
run before foreign_data you would need to update the output of this
test as well, something rather unfriendly.
- Regression coverage is limited (there is nothing done for comments
and default expressions)
- regression tests are added in postgres_fdw. This should be perhaps
the target of another patch so I removed them for now as this is only
a core feature (if I am wrong here don't hesitate). Same remark about
information_schema though, those tests are too fragile as they are.
- Documentation had some issues IMO:
-- A bracket was missing before "<replaceable class="PARAMETER">column_name"...
-- like_option should be clear about what it supports or not, more
precisely that it supports only default expressions and comments
-- some typos and formatting inconsistencies found
- In the case of CREATE TABLE, like_option is bypassed based on the
nature of the object linked, and not based on the nature of the object
created, so for CREATE FOREIGN TABLE, using this argument, I do not
think that we should simply ignore the options not directly supported
but return an error or a warning at least to user (attached patch
returns an ERROR). Documentation needs to reflect that precisely to
let the user know what can be and cannot be done.
After testing the patch, well it does what it is aimed for and it
works. It is somewhat unfortunate that we cannot enforce the name of
columns hidden behind LIKE directly with CREATE, but this would result
in some kludging in the code. It can as well be done simply with ALTER
FOREIGN TABLE.
All those comments result in the patch attached, which I think is in a
state close to committable, so I am marking it as "ready for
committer" (feel free to scream at me if you do not think so). Note
that the patch attached is not using context diffs but git diffs
(really I tried!) because of filterdiff that skipped a block of code
in parse_utilcmd.c.
Regards,
--
Michael
diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
index 1ef4b5e..ecef3c0 100644
--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -19,7 +19,8 @@
<refsynopsisdiv>
<synopsis>
CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name</replaceable> ( [
- <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable> [ OPTIONS ( <replaceable class="PARAMETER">option</replaceable> '<replaceable class="PARAMETER">value</replaceable>' [, ... ] ) ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
+ { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable> [ OPTIONS ( <replaceable class="PARAMETER">option</replaceable> '<replaceable class="PARAMETER">value</replaceable>' [, ... ] ) ] [ COLLATE <replaceable>collation</replaceable> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
+ | LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ] }
[, ... ]
] )
SERVER <replaceable class="parameter">server_name</replaceable>
@@ -31,6 +32,10 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name
{ NOT NULL |
NULL |
DEFAULT <replaceable>default_expr</replaceable> }
+
+<phrase>and <replaceable class="PARAMETER">like_option</replaceable> is:</phrase>
+
+{ INCLUDING | EXCLUDING } { DEFAULTS | COMMENTS | ALL }
</synopsis>
</refsynopsisdiv>
@@ -114,6 +119,29 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="PARAMETER">table_name
</varlistentry>
<varlistentry>
+ <term><literal>LIKE <replaceable>source_table</replaceable> [ <replaceable>like_option</replaceable> ... ]</literal></term>
+ <listitem>
+ <para>
+ The <literal>LIKE</literal> clause specifies a table from which
+ the new foreign table automatically copies all column names and
+ their data types.
+ </para>
+ <para>
+ Default expressions for the copied column definitions will only be
+ copied if <literal>INCLUDING DEFAULTS</literal> is specified.
+ Defaults that call database-modification functions, like
+ <function>nextval</>, create a linkage between the original and
+ new tables. The default behavior is to exclude default expressions,
+ resulting in the copied columns in the new table having null defaults.
+ </para>
+ <para>
+ The <literal>LIKE</literal> clause can also be used to copy columns from
+ views, tables, or composite types.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>NOT NULL</></term>
<listitem>
<para>
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index eb07ca3..aec39b8 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -649,8 +649,8 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
/*
* transformTableLikeClause
*
- * Change the LIKE <srctable> portion of a CREATE TABLE statement into
- * column definitions which recreate the user defined column portions of
+ * Change the LIKE <srctable> portion of a CREATE [FOREIGN] TABLE statement
+ * into column definitions which recreate the user defined column portions of
* <srctable>.
*/
static void
@@ -668,12 +668,6 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
setup_parser_errposition_callback(&pcbstate, cxt->pstate,
table_like_clause->relation->location);
- /* we could support LIKE in many cases, but worry about it another day */
- if (cxt->isforeign)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("LIKE is not supported for creating foreign tables")));
-
relation = relation_openrv(table_like_clause->relation, AccessShareLock);
if (relation->rd_rel->relkind != RELKIND_RELATION &&
@@ -689,6 +683,27 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
cancel_parser_errposition_callback(&pcbstate);
/*
+ * Special handling for foreign tables as only default expressions
+ * and comments are supported.
+ */
+ if (cxt->isforeign)
+ {
+ /* ALL assumes default expressions and comments */
+ if (table_like_clause->options == CREATE_TABLE_LIKE_ALL)
+ table_like_clause->options &= CREATE_TABLE_LIKE_DEFAULTS |
+ CREATE_TABLE_LIKE_COMMENTS;
+
+ /* Other options are not supported */
+ if ((table_like_clause->options & CREATE_TABLE_LIKE_CONSTRAINTS) ||
+ (table_like_clause->options & CREATE_TABLE_LIKE_INDEXES) ||
+ (table_like_clause->options & CREATE_TABLE_LIKE_STORAGE))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("Foreign tables only support default expressions and comments with LIKE clause",
+ RelationGetRelationName(relation))));
+ }
+
+ /*
* Check for privileges
*/
if (relation->rd_rel->relkind == RELKIND_COMPOSITE_TYPE)
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 60506e0..fe8e3ae 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -699,6 +699,47 @@ SELECT * FROM ft1; -- ERROR
ERROR: foreign-data wrapper "dummy" has no handler
EXPLAIN SELECT * FROM ft1; -- ERROR
ERROR: foreign-data wrapper "dummy" has no handler
+-- LIKE CLAUSE
+CREATE TABLE ft2 (a int DEFAULT 0);
+COMMENT ON COLUMN ft2.a IS 'Column a of ft2';
+CREATE FOREIGN TABLE ft2_like_1 (LIKE ft2) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_2 (LIKE ft2 INCLUDING DEFAULTS) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_3 (LIKE ft2 INCLUDING COMMENTS) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_4 (
+ LIKE ft2 INCLUDING INDEXES) SERVER s0; -- ERROR
+ERROR: Foreign tables only support default expressions and comments with LIKE clause
+CREATE FOREIGN TABLE ft2_like_4 (LIKE ft2 INCLUDING ALL) SERVER s0;
+\d+ ft2_like_*
+ Foreign table "public.ft2_like_1"
+ Column | Type | Modifiers | FDW Options | Storage | Stats target | Description
+--------+---------+-----------+-------------+---------+--------------+-------------
+ a | integer | | | plain | |
+Server: s0
+Has OIDs: no
+
+ Foreign table "public.ft2_like_2"
+ Column | Type | Modifiers | FDW Options | Storage | Stats target | Description
+--------+---------+-----------+-------------+---------+--------------+-------------
+ a | integer | default 0 | | plain | |
+Server: s0
+Has OIDs: no
+
+ Foreign table "public.ft2_like_3"
+ Column | Type | Modifiers | FDW Options | Storage | Stats target | Description
+--------+---------+-----------+-------------+---------+--------------+-----------------
+ a | integer | | | plain | | Column a of ft2
+Server: s0
+Has OIDs: no
+
+ Foreign table "public.ft2_like_4"
+ Column | Type | Modifiers | FDW Options | Storage | Stats target | Description
+--------+---------+-----------+-------------+---------+--------------+-----------------
+ a | integer | default 0 | | plain | | Column a of ft2
+Server: s0
+Has OIDs: no
+
+DROP TABLE ft2;
+DROP FOREIGN TABLE ft2_like_1, ft2_like_2, ft2_like_3, ft2_like_4;
-- ALTER FOREIGN TABLE
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index f819eb1..c4dc1f1 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -281,6 +281,19 @@ CREATE INDEX id_ft1_c2 ON ft1 (c2); -- ERROR
SELECT * FROM ft1; -- ERROR
EXPLAIN SELECT * FROM ft1; -- ERROR
+-- LIKE CLAUSE
+CREATE TABLE ft2 (a int DEFAULT 0);
+COMMENT ON COLUMN ft2.a IS 'Column a of ft2';
+CREATE FOREIGN TABLE ft2_like_1 (LIKE ft2) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_2 (LIKE ft2 INCLUDING DEFAULTS) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_3 (LIKE ft2 INCLUDING COMMENTS) SERVER s0;
+CREATE FOREIGN TABLE ft2_like_4 (
+ LIKE ft2 INCLUDING INDEXES) SERVER s0; -- ERROR
+CREATE FOREIGN TABLE ft2_like_4 (LIKE ft2 INCLUDING ALL) SERVER s0;
+\d+ ft2_like_*
+DROP TABLE ft2;
+DROP FOREIGN TABLE ft2_like_1, ft2_like_2, ft2_like_3, ft2_like_4;
+
-- ALTER FOREIGN TABLE
COMMENT ON FOREIGN TABLE ft1 IS 'foreign table';
COMMENT ON FOREIGN TABLE ft1 IS NULL;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
