Re: IMPORT FOREIGN SCHEMA statement

Le lundi 30 juin 2014 16:43:38 Michael Paquier a écrit :
> On Thu, Jun 19, 2014 at 11:00 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> > wrote:
> > >> 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 ?
> > >
> > > Sure. I'll try harder when looking in more details at the patch for
> > > postgres_fdw :)
> >
> > With v2, I have tried randomly some of the scenarios of v1 plus some
> > extras, but was not able to reproduce it.
> >
> > > I'll look into the patch for postgres_fdw later.
> >
> > And here are some comments about it, when applied on top of the other 2
> > patches.
> > 1) Code compiles without warning, regression tests pass.
> > 2) In fetch_remote_tables, the palloc for the parameters should be
> > done after the number of parameters is determined. In the case of
> > IMPORT_ALL, some memory is wasted for nothing.
> > 3) Current code is not able to import default expressions for a table.
> > A little bit of processing with pg_get_expr would be necessary:
> > select pg_catalog.pg_get_expr(adbin, adrelid) from pg_attrdef;
> > There are of course bonus cases like SERIAL columns coming immediately
> > to my mind but it would be possible to determine if a given column is
> > serial with pg_get_serial_sequence.
> > This would be a good addition for the FDW IMO.
> > 4) The same applies of course to the constraint name: CREATE FOREIGN
> > TABLE foobar (a int CONSTRAINT toto NOT NULL) for example.
> > 5) A bonus idea: enable default expression obtention and null/not null
> > switch by default but add options to disable their respective
> > obtention.
> > 6) Defining once PGFDW_IMPORTRESULT_NUMCOLS at the top of
> > postgres_fdw.c without undefining would be perfectly fine.
> > 7) In postgresImportForeignSchema, the palloc calls and the
> > definitions of the variables used to save the results should be done
> > within the for loop.
> > 8) At quick glance, the logic of postgresImportForeignSchema looks
> > awkward... I'll have a second look with a fresher mind later on this
> > one.
>
> While having a second look at the core patch, I have found myself
> re-hacking it, fixing a couple of bugs and adding things that have been
> missing in the former implementation:
> - Deletions of unnecessary structures to simplify code and make it cleaner
> - Fixed a bug related to the management of local schema name. A FDW was
> free to set the schema name where local tables are created, this should not
> be the case.
> - Improved documentation, examples and other things, fixed doc padding for
> example
> - Added some missing stuff in equalfuncs.c and copyfuncs.c
> - Some other things.
> With that, core patch looks pretty nice actually, and I think that we
> should let a committer have a look at this part at least for this CF.
>
> Also, the postgres_fdw portion has been updated based on the previous core
> patch modified, using a version that Ronan sent me, which has addressed the
> remarks I sent before. This patch is still lacking documentation, some
> cleanup, and regression tests are broken, but it can be used to test the
> core feature. I unfortunately don't have much time today but I am sending
> this patch either way to let people play with IMPORT SCHEMA if I don't come
> back to it before.

The regression tests fail because of a typo in pg_type.h: BOOLARRAYOID should
be defined to 1000, not 1003 (which clashes against NAMEARRAYOID).

What do you think should be documented, and where ?

> Regards,

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