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

Lists: pgsql-hackers
From: David Fetter <david(at)fetter(dot)org>
To: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2013-10-08 06:16:56
Message-ID: 20131008061656.GA6300@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Folks,

Please find attached a patch implementing and documenting, to some
extent, $subject. I did this in aid of being able to import SQL
standard catalogs and other entities where a known example could
provide a template for a foreign table.

Should there be errhint()s, too? Should we pile up all such errors
and mention them at the end rather than simply bailing on the first
one?

TBD: regression tests.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Attachment Content-Type Size
foreign_table_like_05.diff text/plain 3.5 KB

From: David Fetter <david(at)fetter(dot)org>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2013-10-15 05:50:04
Message-ID: 20131015055004.GA20846@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Oct 07, 2013 at 11:16:56PM -0700, David Fetter wrote:
> Folks,
>
> Please find attached a patch implementing and documenting, to some
> extent, $subject. I did this in aid of being able to import SQL
> standard catalogs and other entities where a known example could
> provide a template for a foreign table.
>
> Should there be errhint()s, too? Should we pile up all such errors
> and mention them at the end rather than simply bailing on the first
> one?
>
> TBD: regression tests.

Now included: regression tests for disallowed LIKE options.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Attachment Content-Type Size
foreign_table_like_06.diff text/plain 5.8 KB

From: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
To: David Fetter <david(at)fetter(dot)org>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2013-11-24 01:03:18
Message-ID: 52915056.3050801@dalibo.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 10/15/2013 07:50 AM, David Fetter wrote:
> On Mon, Oct 07, 2013 at 11:16:56PM -0700, David Fetter wrote:
>> Folks,
>>
>> Please find attached a patch implementing and documenting, to some
>> extent, $subject. I did this in aid of being able to import SQL
>> standard catalogs and other entities where a known example could
>> provide a template for a foreign table.
>>
>> Should there be errhint()s, too? Should we pile up all such errors
>> and mention them at the end rather than simply bailing on the first
>> one?
>>
>> TBD: regression tests.
> Now included: regression tests for disallowed LIKE options.

I like this patch, but I don't like its implementation at all.

First of all, the documentation doesn't compile:

openjade:ref/create_foreign_table.sgml:124:17:E: end tag for "LISTITEM"
omitted, but OMITTAG NO was specified
openjade:ref/create_foreign_table.sgml:119:4: start tag was here

I fixed that, and then noticed that like_option is not explained like it
is in CREATE TABLE.

Then I got down to the description of the LIKE clause in both pages, and
I noticed the last line of CREATE TABLE, which is "Inapplicable options
(e.g., INCLUDING INDEXES from a view) are ignored.". This is
inconsistent with the behavior of this patch to throw errors for
inapplicable options.

Attached is a patch which corrects and completes the documentation
issues noted above, and also silently ignores inapplicable options. In
addition to reducing patch size, this also allows the use of INCLUDING
ALL. Because these options no longer produce errors, and that's all the
regression tests were looking for, I have removed those tests
(unfortunately leaving none).

Aside from this difference in behavior, I see no fault in this patch.

I am marking this patch as 'returned with feedback' in the commitfest app.

--
Vik

Attachment Content-Type Size
foreign_table_like_20131124.patch text/x-diff 3.5 KB

From: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
To: David Fetter <david(at)fetter(dot)org>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-16 00:07:50
Message-ID: 52D722D6.6000501@dalibo.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 11/24/2013 02:03 AM, Vik Fearing wrote:
> On 10/15/2013 07:50 AM, David Fetter wrote:
>> On Mon, Oct 07, 2013 at 11:16:56PM -0700, David Fetter wrote:
>>> Folks,
>>>
>>> Please find attached a patch implementing and documenting, to some
>>> extent, $subject. I did this in aid of being able to import SQL
>>> standard catalogs and other entities where a known example could
>>> provide a template for a foreign table.
>>>
>>> Should there be errhint()s, too? Should we pile up all such errors
>>> and mention them at the end rather than simply bailing on the first
>>> one?
>>>
>>> TBD: regression tests.
>> Now included: regression tests for disallowed LIKE options.
> I like this patch, but I don't like its implementation at all.
>
> First of all, the documentation doesn't compile:
>
> openjade:ref/create_foreign_table.sgml:124:17:E: end tag for "LISTITEM"
> omitted, but OMITTAG NO was specified
> openjade:ref/create_foreign_table.sgml:119:4: start tag was here
>
>
> I fixed that, and then noticed that like_option is not explained like it
> is in CREATE TABLE.
>
> Then I got down to the description of the LIKE clause in both pages, and
> I noticed the last line of CREATE TABLE, which is "Inapplicable options
> (e.g., INCLUDING INDEXES from a view) are ignored.". This is
> inconsistent with the behavior of this patch to throw errors for
> inapplicable options.
>
> Attached is a patch which corrects and completes the documentation
> issues noted above, and also silently ignores inapplicable options. In
> addition to reducing patch size, this also allows the use of INCLUDING
> ALL. Because these options no longer produce errors, and that's all the
> regression tests were looking for, I have removed those tests
> (unfortunately leaving none).
>
> Aside from this difference in behavior, I see no fault in this patch.
>
> I am marking this patch as 'returned with feedback' in the commitfest app.
>

It looks like this patch got left behind in the previous commitfest.
What is the policy for moving it? Is it too late and will have to wait
until the next commitfest?

https://commitfest.postgresql.org/action/patch_view?id=1254

--
Vik


From: David Fetter <david(at)fetter(dot)org>
To: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-16 00:17:54
Message-ID: 20140116001754.GB31897@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Jan 16, 2014 at 01:07:50AM +0100, Vik Fearing wrote:
> On 11/24/2013 02:03 AM, Vik Fearing wrote:
> > On 10/15/2013 07:50 AM, David Fetter wrote:
> >> On Mon, Oct 07, 2013 at 11:16:56PM -0700, David Fetter wrote:
> >>> Folks,
> >>>
> >>> Please find attached a patch implementing and documenting, to some
> >>> extent, $subject. I did this in aid of being able to import SQL
> >>> standard catalogs and other entities where a known example could
> >>> provide a template for a foreign table.
> >>>
> >>> Should there be errhint()s, too? Should we pile up all such errors
> >>> and mention them at the end rather than simply bailing on the first
> >>> one?
> >>>
> >>> TBD: regression tests.
> >> Now included: regression tests for disallowed LIKE options.
> > I like this patch, but I don't like its implementation at all.
> >
> > First of all, the documentation doesn't compile:
> >
> > openjade:ref/create_foreign_table.sgml:124:17:E: end tag for "LISTITEM"
> > omitted, but OMITTAG NO was specified
> > openjade:ref/create_foreign_table.sgml:119:4: start tag was here
> >
> >
> > I fixed that, and then noticed that like_option is not explained like it
> > is in CREATE TABLE.
> >
> > Then I got down to the description of the LIKE clause in both pages, and
> > I noticed the last line of CREATE TABLE, which is "Inapplicable options
> > (e.g., INCLUDING INDEXES from a view) are ignored.". This is
> > inconsistent with the behavior of this patch to throw errors for
> > inapplicable options.
> >
> > Attached is a patch which corrects and completes the documentation
> > issues noted above, and also silently ignores inapplicable options. In
> > addition to reducing patch size, this also allows the use of INCLUDING
> > ALL. Because these options no longer produce errors, and that's all the
> > regression tests were looking for, I have removed those tests
> > (unfortunately leaving none).
> >
> > Aside from this difference in behavior, I see no fault in this patch.
> >
> > I am marking this patch as 'returned with feedback' in the commitfest app.
> >
>
> It looks like this patch got left behind in the previous commitfest.
> What is the policy for moving it? Is it too late and will have to wait
> until the next commitfest?
>
> https://commitfest.postgresql.org/action/patch_view?id=1254

I think we should still be OK putting it in the current one.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-16 00:21:07
Message-ID: 6156.1389831667@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>> I am marking this patch as 'returned with feedback' in the commitfest app.

> It looks like this patch got left behind in the previous commitfest.
> What is the policy for moving it? Is it too late and will have to wait
> until the next commitfest?

> https://commitfest.postgresql.org/action/patch_view?id=1254

I think you were in error to mark it "returned with feedback", as that
caused everyone to stop paying attention to it in that commitfest.
(And David dropped the ball too, as he should have done something to
bring it back from that state, if it was committable or nearly so.)

I see no reason why you shouldn't move it to the new fest; perhaps
mark it as waiting on author, since really it's his responsibility
to take the next step, ie comment on your version of the patch.

regards, tom lane


From: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: David Fetter <david(at)fetter(dot)org>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-16 00:28:04
Message-ID: 52D72794.20006@dalibo.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 01/16/2014 01:21 AM, Tom Lane wrote:
> Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>>> I am marking this patch as 'returned with feedback' in the commitfest app.
>> It looks like this patch got left behind in the previous commitfest.
>> What is the policy for moving it? Is it too late and will have to wait
>> until the next commitfest?
>> https://commitfest.postgresql.org/action/patch_view?id=1254
> I think you were in error to mark it "returned with feedback", as that
> caused everyone to stop paying attention to it in that commitfest.
> (And David dropped the ball too, as he should have done something to
> bring it back from that state, if it was committable or nearly so.)

I see. Sorry about that.

> I see no reason why you shouldn't move it to the new fest; perhaps
> mark it as waiting on author, since really it's his responsibility
> to take the next step, ie comment on your version of the patch.

Done.

--
Vik


From: David Fetter <david(at)fetter(dot)org>
To: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-25 05:25:01
Message-ID: 20140125052501.GA28649@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sun, Nov 24, 2013 at 02:03:18AM +0100, Vik Fearing wrote:
> On 10/15/2013 07:50 AM, David Fetter wrote:
> > On Mon, Oct 07, 2013 at 11:16:56PM -0700, David Fetter wrote:
> >> Folks,
> >>
> >> Please find attached a patch implementing and documenting, to some
> >> extent, $subject. I did this in aid of being able to import SQL
> >> standard catalogs and other entities where a known example could
> >> provide a template for a foreign table.
> >>
> >> Should there be errhint()s, too? Should we pile up all such errors
> >> and mention them at the end rather than simply bailing on the first
> >> one?
> >>
> >> TBD: regression tests.
> > Now included: regression tests for disallowed LIKE options.
>
> I like this patch, but I don't like its implementation at all.
>
> First of all, the documentation doesn't compile:
>
> openjade:ref/create_foreign_table.sgml:124:17:E: end tag for "LISTITEM"
> omitted, but OMITTAG NO was specified
> openjade:ref/create_foreign_table.sgml:119:4: start tag was here

Fixed.

> I fixed that, and then noticed that like_option is not explained like it
> is in CREATE TABLE.

Also fixed.

> Then I got down to the description of the LIKE clause in both pages, and
> I noticed the last line of CREATE TABLE, which is "Inapplicable options
> (e.g., INCLUDING INDEXES from a view) are ignored.". This is
> inconsistent with the behavior of this patch to throw errors for
> inapplicable options.

Fixed.

Please find attached the next rev :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Attachment Content-Type Size
foreign_table_like_07.diff text/plain 3.5 KB

From: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
To: David Fetter <david(at)fetter(dot)org>
Cc: PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-01-31 17:16:18
Message-ID: 52EBDA62.9020902@dalibo.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 01/25/2014 06:25 AM, David Fetter wrote:
>> I like this patch, but I don't like its implementation at all.
>> >
>> > First of all, the documentation doesn't compile:
>> >
>> > openjade:ref/create_foreign_table.sgml:124:17:E: end tag for "LISTITEM"
>> > omitted, but OMITTAG NO was specified
>> > openjade:ref/create_foreign_table.sgml:119:4: start tag was here
> Fixed.
>
>> > I fixed that, and then noticed that like_option is not explained like it
>> > is in CREATE TABLE.
> Also fixed.
>
>> > Then I got down to the description of the LIKE clause in both pages, and
>> > I noticed the last line of CREATE TABLE, which is "Inapplicable options
>> > (e.g., INCLUDING INDEXES from a view) are ignored.". This is
>> > inconsistent with the behavior of this patch to throw errors for
>> > inapplicable options.
> Fixed.
>
> Please find attached the next rev :)

This version looks committable to me, so I am marking it as such.

--
Vik


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-15 14:14:03
Message-ID: 20140215141403.GA19470@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-01-31 18:16:18 +0100, Vik Fearing wrote:
> On 01/25/2014 06:25 AM, David Fetter wrote:
> > Please find attached the next rev :)
>
> This version looks committable to me, so I am marking it as such.

This doesn't contain a single regression test, I don't see how that's
ok. Marking as waiting on author.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: David Fetter <david(at)fetter(dot)org>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 04:27:09
Message-ID: 20140217042709.GB2420@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Feb 15, 2014 at 03:14:03PM +0100, Andres Freund wrote:
> On 2014-01-31 18:16:18 +0100, Vik Fearing wrote:
> > On 01/25/2014 06:25 AM, David Fetter wrote:
> > > Please find attached the next rev :)
> >
> > This version looks committable to me, so I am marking it as such.
>
> This doesn't contain a single regression test, I don't see how that's
> ok. Marking as waiting on author.

It now contains regression tests. Re-marking.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Attachment Content-Type Size
foreign_table_like_08.diff text/plain 11.5 KB

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: David Fetter <david(at)fetter(dot)org>
Cc: Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 09:28:22
Message-ID: 20140217092822.GP19470@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-02-16 20:27:09 -0800, David Fetter wrote:
> On Sat, Feb 15, 2014 at 03:14:03PM +0100, Andres Freund wrote:
> > On 2014-01-31 18:16:18 +0100, Vik Fearing wrote:
> > > On 01/25/2014 06:25 AM, David Fetter wrote:
> > > > Please find attached the next rev :)
> > >
> > > This version looks committable to me, so I am marking it as such.
> >
> > This doesn't contain a single regression test, I don't see how that's
> > ok. Marking as waiting on author.
>
> It now contains regression tests. Re-marking.

I don't think this really has gone above Needs Review yet.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 14:07:45
Message-ID: CAB7nPqR4cd-WATeM0P+YuuXwTuE+ORq6KG3pDN+cDuF1hX+1wg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

Attachment Content-Type Size
20140217_foreign_like_v9.patch text/x-patch 8.7 KB

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 14:29:47
Message-ID: 20140217142947.GC18388@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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 :(

Uh. What should I then say if a patch is marked as ready for committer
by the author, after it previously had been marked such when it clearly
wasn't? Your review just seems to confirm that it wasn't ready?
If the patch is isn't marked "needs review" in the CF it's less likely
to get timely review. And when a committer looks at the patch it'll just
be determined at not being ready again, making it less likely to get
committed.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 22:22:47
Message-ID: 20140217222247.GI7161@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-17 23:35:35
Message-ID: CAB7nPqTmUo_3668uHJ8s=PFV=BS-PQN60n46yT8tuUoZxGjakw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-02-18 20:59:12
Message-ID: 20140218205912.GA28858@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-02-18 08:35:35 +0900, Michael Paquier wrote:
> 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.

As I said, I am not all that concerned that it's a big problem today,
but imo it's an accident waiting to happen.

I rather wonder if the code shouln't just ensure it's running
transformTableLikeClause() before transformColumnDefinition() by doing
it in a separate loop.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-04-05 15:46:16
Message-ID: 15138.1396712776@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Tue, Feb 18, 2014 at 7:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> 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.

ISTM this is because the proposed feature is wrongheaded. The basic
concept of CREATE TABLE LIKE is that you're copying properties from
another object of the same type. You might or might not want every
property, but there's no question of whether you *could* copy every
property. In contrast, what this is proposing to do is copy properties
from (what might be) a plain table to a foreign table, and those things
aren't even remotely the same kind of object.

It would make sense to me to restrict LIKE to copy from another foreign
table, and then there would be a different set of INCLUDING/EXCLUDING
options that would be relevant (options yes, indexes no, for example).

In any case, I agree with Andres' concern: whether or not it's a bug
currently that this bypasses some of the normal processing, it's a hazard
that can be expected to bite us someday.

regards, tom lane


From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-04-07 20:24:45
Message-ID: 20140407202445.GN4161@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2014-04-05 11:46:16 -0400, Tom Lane wrote:
> ISTM this is because the proposed feature is wrongheaded. The basic
> concept of CREATE TABLE LIKE is that you're copying properties from
> another object of the same type. You might or might not want every
> property, but there's no question of whether you *could* copy every
> property. In contrast, what this is proposing to do is copy properties
> from (what might be) a plain table to a foreign table, and those things
> aren't even remotely the same kind of object.
>
> It would make sense to me to restrict LIKE to copy from another foreign
> table, and then there would be a different set of INCLUDING/EXCLUDING
> options that would be relevant (options yes, indexes no, for example).

I actually think it's quite useful to create a foreign table that's the
same shape as a local table. And the patches approach of refusing to
copy thinks that aren't supported sounds sane to me.
Consider e.g. moving off older partitioned data off to an archiving
server. New local partitions are often created using CREATE TABLE LIKE,
but that's not possible for the foreign ones.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-04-08 00:26:30
Message-ID: CAB7nPqRX-6P5+iy-mktnP_Mk0_ThVcCxseYcp1C+QFvSEgL8Nw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Tue, Apr 8, 2014 at 5:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-04-05 11:46:16 -0400, Tom Lane wrote:
>> ISTM this is because the proposed feature is wrongheaded. The basic
>> concept of CREATE TABLE LIKE is that you're copying properties from
>> another object of the same type. You might or might not want every
>> property, but there's no question of whether you *could* copy every
>> property. In contrast, what this is proposing to do is copy properties
>> from (what might be) a plain table to a foreign table, and those things
>> aren't even remotely the same kind of object.
>>
>> It would make sense to me to restrict LIKE to copy from another foreign
>> table, and then there would be a different set of INCLUDING/EXCLUDING
>> options that would be relevant (options yes, indexes no, for example).
>
> I actually think it's quite useful to create a foreign table that's the
> same shape as a local table. And the patches approach of refusing to
> copy thinks that aren't supported sounds sane to me.
This could be improved as well: it would be useful to be able to copy
the column options of another foreign table.

> Consider e.g. moving off older partitioned data off to an archiving
> server. New local partitions are often created using CREATE TABLE LIKE,
> but that's not possible for the foreign ones.
Definitely a use case.
--
Michael


From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, David Fetter <david(at)fetter(dot)org>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-04-08 15:52:50
Message-ID: CA+TgmoYQ3bCuOW4Y5_x_zXh+5t3rCL9Ye4YpK_5rszw7wcR0Zg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Apr 7, 2014 at 4:24 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-04-05 11:46:16 -0400, Tom Lane wrote:
>> ISTM this is because the proposed feature is wrongheaded. The basic
>> concept of CREATE TABLE LIKE is that you're copying properties from
>> another object of the same type. You might or might not want every
>> property, but there's no question of whether you *could* copy every
>> property. In contrast, what this is proposing to do is copy properties
>> from (what might be) a plain table to a foreign table, and those things
>> aren't even remotely the same kind of object.
>>
>> It would make sense to me to restrict LIKE to copy from another foreign
>> table, and then there would be a different set of INCLUDING/EXCLUDING
>> options that would be relevant (options yes, indexes no, for example).
>
> I actually think it's quite useful to create a foreign table that's the
> same shape as a local table. And the patches approach of refusing to
> copy thinks that aren't supported sounds sane to me.
> Consider e.g. moving off older partitioned data off to an archiving
> server. New local partitions are often created using CREATE TABLE LIKE,
> but that's not possible for the foreign ones.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: CREATE FOREIGN TABLE ( ... LIKE ... )
Date: 2014-04-23 08:46:04
Message-ID: 53577DCC.3050003@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

(2014/04/08 9:26), Michael Paquier wrote:
> On Tue, Apr 8, 2014 at 5:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2014-04-05 11:46:16 -0400, Tom Lane wrote:
>>> ISTM this is because the proposed feature is wrongheaded. The basic
>>> concept of CREATE TABLE LIKE is that you're copying properties from
>>> another object of the same type. You might or might not want every
>>> property, but there's no question of whether you *could* copy every
>>> property. In contrast, what this is proposing to do is copy properties
>>> from (what might be) a plain table to a foreign table, and those things
>>> aren't even remotely the same kind of object.
>>>
>>> It would make sense to me to restrict LIKE to copy from another foreign
>>> table, and then there would be a different set of INCLUDING/EXCLUDING
>>> options that would be relevant (options yes, indexes no, for example).
>>
>> I actually think it's quite useful to create a foreign table that's the
>> same shape as a local table. And the patches approach of refusing to
>> copy thinks that aren't supported sounds sane to me.
> This could be improved as well: it would be useful to be able to copy
> the column options of another foreign table.

Yes, I think so, too. But to think of validating generic column/table
options, I think we would probably need to restrict LIKE to copy from
another foreign table maybe using the same FDW. So, I'd like to vote
for Tom's idea.

Thanks,

Best regards,
Etsuro Fujita