Re: CREATE FOREIGN TABLE ( ... LIKE ... )

On Mon, Feb 17, 2014 at 6:28 PM, Andres Freund <andres(at)2ndquadrant(dot)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