On Tue, Feb 18, 2014 at 7:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-17 23:07:45 +0900, Michael Paquier wrote:
>> 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:
>
> David just pinged me and tricked me into having a quick look :)
>
> Unless I miss something this possibly allows column definition to slip
> by that shouldn't because normally all fdw column definitions are passed
> through transformColumnDefinition() which does some checks, but the
> copied ones aren't.
> I haven't looked long enough to see whether that's currently
> problematic, but even if not, it's sure a trap waiting to spring.
transformColumnDefinition contains checks about serial and constraints
mainly. The only thing that could be problematic IMO is the process
done exclusively for foreign tables which is the creation of some
ALTER FOREIGN TABLE ALTER COLUMN commands when per-column options are
detected, something that is not passed to a like'd table with this
patch. This may meritate a comment in the code.
Actually after more thinking I think that it would make sense to have
another INCLUDING/EXCLUDING option for foreign tables: OPTIONS to pass
the column options when link is done from another foreign table. This
should be another patch though.
Regards,
--
Michael