Re: IMPORT FOREIGN SCHEMA statement

Le lundi 16 juin 2014 16:07:51 Michael Paquier a écrit :
> On Sat, May 24, 2014 at 5:08 AM, Ronan Dunklau <ronan(dot)dunklau(at)dalibo(dot)com>
wrote:
> > Hello,
> >
> > Since my last proposal didn't get any strong rebuttal, please find
> > attached a more complete version of the IMPORT FOREIGN SCHEMA statement.
> >
> > I tried to follow the SQL-MED specification as closely as possible.
> >
> > This adds discoverability to foreign servers. The structure of the
> > statement as I understand it is simple enough:
> >
> > IMPORT FOREIGN SCHEMA remote_schema FROM SERVER some_server [ (LIMIT TO |
> > EXCEPT) table_list ] INTO local_schema.
> >
> > The import_foreign_schema patch adds the infrastructure, and a new FDW
> > routine:
> >
> > typedef List *(*ImportForeignSchema_function) (ForeignServer *server,
> > ImportForeignSchemaStmt * parsetree);
> >
> > This routine must return a list of CreateForeignTableStmt mirroring
> > whatever tables were found on the remote side, which will then be
> > executed.
> >
> > The import_foreign_schema_postgres_fdw patch proposes an implementation of
> > this API for postgres_fdw. It will import a foreign schema using the right
> > types as well as nullable information.
> >
> > Regarding documentation, I don't really know where it should have been
> > put. If I missed something, let me know and I'll try to correct it.
>
> I have been playing a bit with this patch... And got a couple of comments.
>
> 1) Perhaps somebody could guide me to a better spec but by looking at
> the spec here => http://farrago.sourceforge.net/design/sqlmed.html, it
> seems that the FROM SERVER clause is misplaced, it should be placed
> after the table list.

You're right, I misplaced it. Corrected in the attached patch.
The spec I'm using is the SQL:201X linked from this page of the wiki:
http://wiki.postgresql.org/wiki/Developer_FAQ#Where_can_I_get_a_copy_of_the_SQL_standards.3F

> 2) The query I am seeing on this spec offers the possiblitily to query
> TABLE_NAME LIKE 'pattern'. Is this part of the spec as well? If yes,
> are you planning to add it as well. I imagine that this could be quite
> handy for users importing from a foreign schema tables that have the
> same prefix name for example. ImportForeignSchemaRestrictionType could
> be extended with an IMPORT_LIKE type. Extending a bit the spec...
> There could be a list of LIKE patterns, this matches more your code by
> adding all of them in table_names. I am not voting to add TABLE_NAME
> in the list of reserved keywords though, so something like "TABLE LIKE
> 'pattern'" would be nice.

From the spec I have been reading, the import qualification is defined as :

<table name list> ::= <table name> [ { <comma> <table name> }... ]

where table name is defined as a simple table name, without any qualifier.

It seems to me the example you linked extended the standard with this
particular implementation. I don't think it would be a problem to add such a
syntax later. In the meantime, FDW implementors can abuse the specification by
implementing a DSL in the table name, for example: LIMIT TO ("LIKE
'pattern*'");

> 3) This has been already raised in this thread, but something missing
> with this patch is the possiblity to pass options when executing an
> import. This could allow a FDW to do more fine-grained operations on
> the underlying objects of the table. There would be two level of
> options: table level and schema level. For example, with the current
> feature it is not possible to rename imported tables on-the-fly. I
> imagine that this would be useful.

The attached patch implements options. Now, we should think about what options
may be desirable for postgres_fdw :)

I don't understand your point about table-level and schema-level options,
though. There is no concept of "foreign schema" allowing to set options on it,
AFAIK.

Renaming imported tables on the fly should, IMO, be done after the import
statement. Since it is transactional, nothing prevents you from running the
ALTER TABLE statements after that.

> 4) Something not mandatory with the core patch but always nice to
> have: tab completion for this command in psql.

I will look into it.

> 5) The error message returned to user when import fails because of a
> missing type is rather unfriendly. Let's imagine with two nodes and
> postgres_fdw... Node 1:
> create type toto as (a int, b int);
> create table toto_tab (a toto);
> Node 2:
> =# import foreign schema public from server postgres_server into public;
> ERROR: 42704: type "public.toto" does not exist
> LOCATION: parseTypeString, parse_type.c:797
> I would rather imagine something like "IMPORT failed because of
> missing type defined on remote but not locally".

I simply prepended the suggested error message to the previous one:

# IMPORT FOREIGN SCHEMA import_source FROM SERVER remote1 INTO
import_destination;
ERROR: IMPORT of table t1 failed because of missing type defined on remote but
not locally: type "public.typ1" does not exist
Time: 11,305 ms

> 6) Import does not fail if a table specified in LIMIT TO is not
> defined on remote-side:
> =# import foreign schema public from server postgres_server limit to
> (tab_not_there) into public;
> IMPORT FOREIGN SCHEMA
> =# \d
> No relations found.

Thanks, this is corrected in the attached patch. The error detection code is
O(n²), but I assumed LIMIT TO clauses should be small enough for it not to be
a problem.

> 7) A small thing: code comments do not respect the project format:
> http://www.postgresql.org/docs/9.1/interactive/source-format.html

This should be fixed too, sorry for the inconvenience.

> 8) In fetch_remote_tables(at)postgres_fdw(dot)c, you are missing materialized
> views: WHERE relkind IN ('r', 'v', 'f')

Fixed in the attached patch.

> 9) The changes in pg_type.h adding new OID defines should be a
> separate patch IMO

It has been broken into its separate patch.

> 10) Code has many whitespaces.

I hope this is corrected in the attached patch, if not, could you please point
me at specific examples ?

> 11) Sometimes the import goes strangely:
> 11-1) After some testing I noticed that tables with incorrect names
> were imported when using LIMIT TO. For example on remote a table
> called "ab" is present, IMPORT SCHEMA LIMIT TO 'other_ab' created a
> local entry called "other_ab" while it should create nothing
> 11-2) Tables with empty names can be created locally. On foreign server:
> =# \d
> List of relations
> Schema | Name | Type | Owner
> --------+----------+-------+--------
> public | aa | table | ioltas
> public | toto_tab | table | ioltas
> (2 rows)
> On local node:
> =# import foreign schema public from server postgres_server limit to
> (toto_tab, "NULL", aa) into public;
> -- (Forget about NULL here, I could reproduce it without as well)
> IMPORT FOREIGN SCHEMA
> Time: 13.589 ms
> =# \d
> List of relations
> Schema | Name | Type | Owner
> --------+----------+---------------+--------
> public | | foreign table | ioltas
> public | toto_tab | foreign table | ioltas
> (2 rows)
> Some more testing showed me that as well:
> =# \d
>
> List
> of relations
> Schema |
> Name
>
> | Type |
>
> Owner
> --------+-------------------------------------------------------------------
> ----------------------------------------------------------------------------
> ----------------------------------------------------------------------------
> -----------------------------------+---------------+-------- public |
>
> | foreign table |
>
> ioltas
> public | toto_tab
>
> | foreign table |
>
> ioltas
> public |F\x7F\x7
> F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7
> F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7
> F\x7F\x7F\x7F\x7F\x7F\x7F
> | foreign table | ioltas
> \x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7

This seems to be related to re-using the table-name between invocations. The
attached patch should fix point 2. As for point 1, I don't know the cause for
it. Do you have a reproducible test case ?

> That's all I have for now.

Thank you very much for this review.

> Regards,

--
Ronan Dunklau
http://dalibo.com - http://dalibo.org