Re: CREATE FOREGIN TABLE LACUNA

Folks,

This is for 9.3, of course.

I noticed that CREATE FOREIGN TABLE (LIKE some_table) doesn't work. I
believe it should, as it would:

- Remove a POLA violation
- Make data loading into an extant table even easier, especially if
there need to be filtering or other cleanup steps

Come to think of it, which CREATE TABLE options are inappropriate to
CREATE FOREIGN TABLE?

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

On Tue, Mar 13, 2012 at 08:24:47AM -0700, David Fetter wrote:
> Folks,
>
> This is for 9.3, of course.
>
> I noticed that CREATE FOREIGN TABLE (LIKE some_table) doesn't work. I
> believe it should, as it would:
>
> - Remove a POLA violation
> - Make data loading into an extant table even easier, especially if
> there need to be filtering or other cleanup steps
>
> Come to think of it, which CREATE TABLE options are inappropriate to
> CREATE FOREIGN TABLE?
>
> Cheers,
> David.

Here's a WIP patch (lots of cut/paste, no docs, no tests), but it does
work. Still to do in addition: decide whether ALTER FOREIGN TABLE
should also handle LIKE.

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

On Wed, Mar 14, 2012 at 8:28 AM, David Fetter <david(at)fetter(dot)org> wrote:
> On Tue, Mar 13, 2012 at 08:24:47AM -0700, David Fetter wrote:
>> Folks,
>>
>> This is for 9.3, of course.
>>
>> I noticed that CREATE FOREIGN TABLE (LIKE some_table) doesn't work.  I
>> believe it should, as it would:
>>
>> - Remove a POLA violation
>> - Make data loading into an extant table even easier, especially if
>>   there need to be filtering or other cleanup steps
>>
>> Come to think of it, which CREATE TABLE options are inappropriate to
>> CREATE FOREIGN TABLE?
>
> Here's a WIP patch (lots of cut/paste, no docs, no tests), but it does
> work.  Still to do in addition: decide whether ALTER FOREIGN TABLE
> should also handle LIKE.

I think that instead of inventing new grammar productions and a new
node type for this, you should just reuse the existing productions for
LIKE clauses and then reject invalid options during parse analysis.
INCLUDING COMMENTS would be OK, but the the rest are no good.

I'd actually like to see us allow foreign tables to have constraints.
Obviously, we can't enforce constraints on remote data, but the point
would be allow the system administrator to supply the query planner
with enough knowledge to make constraint exclusion work. The fact
that you can't make that work today is a major gap, IMV.

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

On Wed, Mar 14, 2012 at 08:53:17AM -0400, Robert Haas wrote:
> On Wed, Mar 14, 2012 at 8:28 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > On Tue, Mar 13, 2012 at 08:24:47AM -0700, David Fetter wrote:
> >> Folks,
> >>
> >> This is for 9.3, of course.
> >>
> >> I noticed that CREATE FOREIGN TABLE (LIKE some_table) doesn't work.  I
> >> believe it should, as it would:
> >>
> >> - Remove a POLA violation
> >> - Make data loading into an extant table even easier, especially if
> >>   there need to be filtering or other cleanup steps
> >>
> >> Come to think of it, which CREATE TABLE options are inappropriate to
> >> CREATE FOREIGN TABLE?
> >
> > Here's a WIP patch (lots of cut/paste, no docs, no tests), but it does
> > work.  Still to do in addition: decide whether ALTER FOREIGN TABLE
> > should also handle LIKE.
>
> I think that instead of inventing new grammar productions and a new
> node type for this, you should just reuse the existing productions for
> LIKE clauses and then reject invalid options during parse analysis.

OK. Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
submit that as a separate patch?

> INCLUDING COMMENTS would be OK, but the the rest are no good.

At least for now. I can see FDWs in the future that would delegate
the decision to the remote side, and if the remote side happens to be
PostgreSQL, a lot of those delegations could be in force. Of course,
this would either create a dependency that would need to be tracked in
the other node or not be able to guarantee the durability of DDL, the
latter being the current situation. I suspect there would be use
cases for each.

> I'd actually like to see us allow foreign tables to have constraints.

So would I :)

> Obviously, we can't enforce constraints on remote data, but the point
> would be allow the system administrator to supply the query planner
> with enough knowledge to make constraint exclusion work. The fact
> that you can't make that work today is a major gap, IMV.

I didn't do INHERITS because most FDWs won't ever have that concept,
i.e. aren't PostgreSQL. Are you thinking about this as a general way
to handle remote partitioned tables?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Mar 14, 2012 at 8:28 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> Here's a WIP patch (lots of cut/paste, no docs, no tests), but it does
>> work. Still to do in addition: decide whether ALTER FOREIGN TABLE
>> should also handle LIKE.

> I think that instead of inventing new grammar productions and a new
> node type for this, you should just reuse the existing productions for
> LIKE clauses and then reject invalid options during parse analysis.

+1; in this approach, adding more features will make it worse not better.

> I'd actually like to see us allow foreign tables to have constraints.
> Obviously, we can't enforce constraints on remote data, but the point
> would be allow the system administrator to supply the query planner
> with enough knowledge to make constraint exclusion work. The fact
> that you can't make that work today is a major gap, IMV.

Hm. That opinion seems to me to connect to the recently-posted patch to
make contrib/file_fdw enforce NOT NULL constraints. Should we instead
have the position that constraints declared for foreign tables are
statements that we can take on faith, and it's the user's fault if they
are wrong?

regards, tom lane

On Wed, Mar 14, 2012 at 10:27:28AM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Mar 14, 2012 at 8:28 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> Here's a WIP patch (lots of cut/paste, no docs, no tests), but it does
> >> work. Still to do in addition: decide whether ALTER FOREIGN TABLE
> >> should also handle LIKE.
>
> > I think that instead of inventing new grammar productions and a new
> > node type for this, you should just reuse the existing productions for
> > LIKE clauses and then reject invalid options during parse analysis.
>
> +1; in this approach, adding more features will make it worse not better.

OK :)

> > I'd actually like to see us allow foreign tables to have constraints.
> > Obviously, we can't enforce constraints on remote data, but the point
> > would be allow the system administrator to supply the query planner
> > with enough knowledge to make constraint exclusion work. The fact
> > that you can't make that work today is a major gap, IMV.
>
> Hm. That opinion seems to me to connect to the recently-posted
> patch to make contrib/file_fdw enforce NOT NULL constraints. Should
> we instead have the position that constraints declared for foreign
> tables are statements that we can take on faith, and it's the user's
> fault if they are wrong?

I think that's something FDWs need to be able to communicate to
PostgreSQL. For example, something talking to another PostgreSQL
would (potentially, anyhow) have access to deep knowledge of the
remote side, but file_fdw would only have best efforts even for clever
things like statistics.

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

David Fetter <david(at)fetter(dot)org> writes:
> I didn't do INHERITS because most FDWs won't ever have that concept,
> i.e. aren't PostgreSQL.

What's that have to do with it? Inheritance would be a local
association of tables, having nothing to do with what the remote end is.
IOW, if c inherits from p, that means to scan c as well in "SELECT FROM
p". We can do this regardless of whether c or p or both are foreign
tables.

regards, tom lane

David Fetter <david(at)fetter(dot)org> writes:
> On Wed, Mar 14, 2012 at 10:27:28AM -0400, Tom Lane wrote:
>> Hm. That opinion seems to me to connect to the recently-posted
>> patch to make contrib/file_fdw enforce NOT NULL constraints. Should
>> we instead have the position that constraints declared for foreign
>> tables are statements that we can take on faith, and it's the user's
>> fault if they are wrong?

> I think that's something FDWs need to be able to communicate to
> PostgreSQL. For example, something talking to another PostgreSQL
> would (potentially, anyhow) have access to deep knowledge of the
> remote side, but file_fdw would only have best efforts even for clever
> things like statistics.

I think we're talking at cross-purposes. What you're saying seems to
assume that it's the system's responsibility to do something about a
constraint declared on a foreign table. What I'm suggesting is that
maybe it isn't. A constraint, ordinarily, would be enforced against
table *updates*, and then just assumed valid during reads. In the case
of a foreign table, we can't enforce constraints during updates because
we don't have control of all updates. Should we ignore declared
constraints because they're not necessarily true? Should we assume on
faith that they're true? The posted patch for file_fdw takes the
approach of silently filtering out rows for which they're not true,
which is not obviously the right thing either --- quite aside from
whether that's a sane semantics, it's not going to scale to foreign key
constraints, and even for simple NOT NULL and CHECK constraints it
results in a runtime penalty on selects, which is not what people would
expect from a constraint.

regards, tom lane

On Wed, Mar 14, 2012 at 11:29:14AM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
> > On Wed, Mar 14, 2012 at 10:27:28AM -0400, Tom Lane wrote:
> >> Hm. That opinion seems to me to connect to the recently-posted
> >> patch to make contrib/file_fdw enforce NOT NULL constraints.
> >> Should we instead have the position that constraints declared for
> >> foreign tables are statements that we can take on faith, and it's
> >> the user's fault if they are wrong?
>
> > I think that's something FDWs need to be able to communicate to
> > PostgreSQL. For example, something talking to another PostgreSQL
> > would (potentially, anyhow) have access to deep knowledge of the
> > remote side, but file_fdw would only have best efforts even for
> > clever things like statistics.
>
> I think we're talking at cross-purposes. What you're saying seems
> to assume that it's the system's responsibility to do something
> about a constraint declared on a foreign table. What I'm suggesting
> is that maybe it isn't.

Actually, I'm suggesting that this behavior needs to be controlled,
not system-wide, but per FDW, and eventually per server, table and
column.

> A constraint, ordinarily, would be enforced against table *updates*,
> and then just assumed valid during reads. In the case of a foreign
> table, we can't enforce constraints during updates because we don't
> have control of all updates.

I think that the situation will become a bit more nuanced than that.
A FDW could delegate constraints to the remote side, and in principle,
the remote side could inform PostgreSQL of all types of changes (DML,
DCL, DDL).

> Should we ignore declared constraints because they're not
> necessarily true? Should we assume on faith that they're true?

Neither. We should instead have ways for FDWs to say which
constraints are local-only, and which presumed correct on the remote
side. If they lie when asserting the latter, that's pilot error.

> The posted patch for file_fdw takes the approach of silently
> filtering out rows for which they're not true, which is not
> obviously the right thing either --- quite aside from whether that's
> a sane semantics,

It clearly is for the author's use case. Other use cases will differ.

> it's not going to scale to foreign key constraints, and even for
> simple NOT NULL and CHECK constraints it results in a runtime
> penalty on selects, which is not what people would expect from a
> constraint.

If people expect FK constraints on, say, a Twitter feed, they're
riding for a very hard fall. If they expect them on a system with
several PostgreSQL nodes in it, that could very well be reasonable.

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

David Fetter <david(at)fetter(dot)org> writes:
> On Wed, Mar 14, 2012 at 11:29:14AM -0400, Tom Lane wrote:
>> The posted patch for file_fdw takes the approach of silently
>> filtering out rows for which they're not true, which is not
>> obviously the right thing either --- quite aside from whether that's
>> a sane semantics,

> It clearly is for the author's use case. Other use cases will differ.

You're assuming facts not in evidence. Fujita-san posted that patch not
because he had any use case one way or the other, but because he read
something in fdwhandler.sgml that made him think it was required to
avoid planner malfunctions. (Actually it is not, at the moment, since
we don't do any optimizations based on NOT NULL properties; but we might
in future.) The question on the table is precisely whether believing a
contrary-to-fact NOT NULL assertion would constitute planner malfunction
or user error.

In general, the approach you're sketching towards foreign constraints
seems to me to be 100% overdesign with no basis in known user
requirements. We have a list longer than my arm of things that are
more pressing than doing anything like that.

regards, tom lane

On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> I think that instead of inventing new grammar productions and a new
>> node type for this, you should just reuse the existing productions for
>> LIKE clauses and then reject invalid options during parse analysis.
>
> OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> submit that as a separate patch?

I don't see any reason to do that. I merely meant that you could
reuse TableLikeClause or maybe even TableElement in the grammer for
CreateForeignTableStmt.

>> INCLUDING COMMENTS would be OK, but the the rest are no good.
>
> At least for now.  I can see FDWs in the future that would delegate
> the decision to the remote side, and if the remote side happens to be
> PostgreSQL, a lot of those delegations could be in force.  Of course,
> this would either create a dependency that would need to be tracked in
> the other node or not be able to guarantee the durability of DDL, the
> latter being the current situation.  I suspect there would be use
> cases for each.

What's relevant for LIKE is whether we want to create constraints,
defaults, comments, etc. on the *local* side, not the remote side -
and that has nothing do with with the particular choice of FDW in use.

I don't think we should conflate the local and remote sides. Even if
a foreign table refers to a remote table that has comments on its
columns, there's no rule that the comments on the foreign side must
match up with the comments on the local side, and in fact I think that
in general we want to keep those concepts clearly distinct. There's
no guarantee that the two systems are controlled by the same DBA, and
they might each have their own choice words about those columns. IOW,
even if we had the ability to keep those things synchronized, we
shouldn't do it, or at least not by default.

>> Obviously, we can't enforce constraints on remote data, but the point
>> would be allow the system administrator to supply the query planner
>> with enough knowledge to make constraint exclusion work.  The fact
>> that you can't make that work today is a major gap, IMV.
>
> I didn't do INHERITS because most FDWs won't ever have that concept,
> i.e. aren't PostgreSQL.  Are you thinking about this as a general way
> to handle remote partitioned tables?

The original foreign table patch included constraints and the ability
to inherit back and forth between regular tables and foreign tables.
I ripped all that out before committing because it wasn't sufficiently
well thought-out, but I'm not convinced it's something we never want
to do. Either way, constraint exclusion can also be used in other
scenarios, like a UNION ALL view over several foreign tables.

And yes, I am thinking about remote partitioned tables. :-)

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

On 14/03/2012 16:47, David Fetter wrote:
> On Wed, Mar 14, 2012 at 11:29:14AM -0400, Tom Lane wrote:
>> David Fetter <david(at)fetter(dot)org> writes:
>>> On Wed, Mar 14, 2012 at 10:27:28AM -0400, Tom Lane wrote:
>>>> Hm. That opinion seems to me to connect to the recently-posted
>>>> patch to make contrib/file_fdw enforce NOT NULL constraints.
>>>> Should we instead have the position that constraints declared for
>>>> foreign tables are statements that we can take on faith, and it's
>>>> the user's fault if they are wrong?
>>
>>> I think that's something FDWs need to be able to communicate to
>>> PostgreSQL. For example, something talking to another PostgreSQL
>>> would (potentially, anyhow) have access to deep knowledge of the
>>> remote side, but file_fdw would only have best efforts even for
>>> clever things like statistics.
>>
>> I think we're talking at cross-purposes. What you're saying seems
>> to assume that it's the system's responsibility to do something
>> about a constraint declared on a foreign table. What I'm suggesting
>> is that maybe it isn't.
>
> Actually, I'm suggesting that this behavior needs to be controlled,
> not system-wide, but per FDW, and eventually per server, table and
> column.

>> A constraint, ordinarily, would be enforced against table *updates*,
>> and then just assumed valid during reads. In the case of a foreign
>> table, we can't enforce constraints during updates because we don't
>> have control of all updates.
>
> I think that the situation will become a bit more nuanced than that.
> A FDW could delegate constraints to the remote side, and in principle,
> the remote side could inform PostgreSQL of all types of changes (DML,
> DCL, DDL).
>
>> Should we ignore declared constraints because they're not
>> necessarily true? Should we assume on faith that they're true?
>
> Neither. We should instead have ways for FDWs to say which
> constraints are local-only, and which presumed correct on the remote
> side. If they lie when asserting the latter, that's pilot error.
>

I don't understand what value would that bring. Do you propose that, if
a FDW declares a constraint as local-only, the planner should ignore
them but when declared as remote, it could use this information ?

Let me describe a simple use case we have in one of our web
applications, which would benefit from foreign keys on foreign tables.

The application has users, stored in a users table, which can upload
files. The files are stored on the server's filesystem, using one folder
per user, named after the user_id.

Ex:

/
1/
myfile.png
2/
myotherfile.png

This filesystem is accessed using the StructuredFS FDW, which maps a
file system tree to a set of columns corresponding to parts of the file
path: every file which path matches the pattern results in a row. Using
the aforementioned structure, the foreign table would have an user_id
column, and a filename column.

Now, the FDW itself cannot know that the foreign key will be enforced,
but as the application developer, I know that every directory will be
named after an user_id.

Allowing foreign keys on such a foreign table would allow us to describe
the model more precisely in PostgreSQL, and external tools could use
this knowledge too, even if PostgreSQL completely ignore them.
Especially ORMs relying on foreign keys to determine join conditions
between tables.

On the other hand, should foreign keys referencing a foreign table be
allowed too ? From a foreign table to another, from a local table to a
foreign table ?

Regards,

--
Ronan Dunklau

On Wed, Mar 14, 2012 at 12:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> David Fetter <david(at)fetter(dot)org> writes:
>> On Wed, Mar 14, 2012 at 11:29:14AM -0400, Tom Lane wrote:
>>> The posted patch for file_fdw takes the approach of silently
>>> filtering out rows for which they're not true, which is not
>>> obviously the right thing either --- quite aside from whether that's
>>> a sane semantics,
>
>> It clearly is for the author's use case.  Other use cases will differ.
>
> You're assuming facts not in evidence.  Fujita-san posted that patch not
> because he had any use case one way or the other, but because he read
> something in fdwhandler.sgml that made him think it was required to
> avoid planner malfunctions.  (Actually it is not, at the moment, since
> we don't do any optimizations based on NOT NULL properties; but we might
> in future.)  The question on the table is precisely whether believing a
> contrary-to-fact NOT NULL assertion would constitute planner malfunction
> or user error.

+1 for user error. I think at some point I had taken the view that
perhaps the FDW should check the data it's emitting against the NOT
NULL constraints, but that would imply that we ought to cross-check
CHECK constraints as well, once we have those, which sounds
unreasonably expensive. So defining the constraint as a promise by
the user seems best.

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

On ons, 2012-03-14 at 10:27 -0400, Tom Lane wrote:
> That opinion seems to me to connect to the recently-posted patch to
> make contrib/file_fdw enforce NOT NULL constraints. Should we instead
> have the position that constraints declared for foreign tables are
> statements that we can take on faith, and it's the user's fault if
> they are wrong?

We should look into the NOT ENFORCED stuff for constraints in SQL:2011.
Then we can have both, and both for regular and foreign tables.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On ons, 2012-03-14 at 10:27 -0400, Tom Lane wrote:
>> That opinion seems to me to connect to the recently-posted patch to
>> make contrib/file_fdw enforce NOT NULL constraints. Should we instead
>> have the position that constraints declared for foreign tables are
>> statements that we can take on faith, and it's the user's fault if
>> they are wrong?

> We should look into the NOT ENFORCED stuff for constraints in SQL:2011.
> Then we can have both, and both for regular and foreign tables.

Have both what? The key point here is that we *can't* enforce
constraints on foreign tables, at least not with anything like the
semantics SQL constraints normally have. Ignoring that point leads
to nonsensical conclusions.

Declaring a foreign constraint as NOT ENFORCED might be a reasonable
thing to do, but it doesn't help us decide what to do when that clause
isn't attached.

On reflection I don't see anything much wrong with the "if you lied
about the constraint it's your fault that things broke" position.
It seems quite comparable to the fact that we take the user's assertions
on faith as to the number and data types of the columns in a foreign
table.

regards, tom lane

On 03/14/2012 04:44 PM, Tom Lane wrote:
> Peter Eisentraut<peter_e(at)gmx(dot)net> writes:
>> On ons, 2012-03-14 at 10:27 -0400, Tom Lane wrote:
>>> That opinion seems to me to connect to the recently-posted patch to
>>> make contrib/file_fdw enforce NOT NULL constraints. Should we instead
>>> have the position that constraints declared for foreign tables are
>>> statements that we can take on faith, and it's the user's fault if
>>> they are wrong?
>> We should look into the NOT ENFORCED stuff for constraints in SQL:2011.
>> Then we can have both, and both for regular and foreign tables.
> Have both what? The key point here is that we *can't* enforce
> constraints on foreign tables, at least not with anything like the
> semantics SQL constraints normally have. Ignoring that point leads
> to nonsensical conclusions.
>
> Declaring a foreign constraint as NOT ENFORCED might be a reasonable
> thing to do, but it doesn't help us decide what to do when that clause
> isn't attached.
>
> On reflection I don't see anything much wrong with the "if you lied
> about the constraint it's your fault that things broke" position.
> It seems quite comparable to the fact that we take the user's assertions
> on faith as to the number and data types of the columns in a foreign
> table.
>
>

Maybe we should say that for foreign tables NOT ENFORCED is implied.
That seems to amount to much the same thing.

cheers

andrew

On ons, 2012-03-14 at 16:44 -0400, Tom Lane wrote:
> On reflection I don't see anything much wrong with the "if you lied
> about the constraint it's your fault that things broke" position.
> It seems quite comparable to the fact that we take the user's
> assertions on faith as to the number and data types of the columns in
> a foreign table.

We do enforce the data types of a foreign table. We can't ensure that
the data that a foreign table "contains" is valid at any moment, but
when we read the data and interact with it in SQL, we reject it if it's
not valid. Similarly, one could conceivably apply not-null and check
constraints as the data is read, which is exactly what the other patch
you referred to proposes. And I think we must do it that way, otherwise
check constraints on domains and check constraints on tables would
behave quite differently.

So if we want to have fake constraints on foreign tables, I think we
should require the NOT ENFORCED decoration or something similar, unless
the FDW signals that it can enforce the constraint.

On Wed, Mar 14, 2012 at 5:14 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On ons, 2012-03-14 at 16:44 -0400, Tom Lane wrote:
>> On reflection I don't see anything much wrong with the "if you lied
>> about the constraint it's your fault that things broke" position.
>> It seems quite comparable to the fact that we take the user's
>> assertions on faith as to the number and data types of the columns in
>> a foreign table.
>
> We do enforce the data types of a foreign table.  We can't ensure that
> the data that a foreign table "contains" is valid at any moment, but
> when we read the data and interact with it in SQL, we reject it if it's
> not valid. Similarly, one could conceivably apply not-null and check
> constraints as the data is read, which is exactly what the other patch
> you referred to proposes.  And I think we must do it that way, otherwise
> check constraints on domains and check constraints on tables would
> behave quite differently.
>
> So if we want to have fake constraints on foreign tables, I think we
> should require the NOT ENFORCED decoration or something similar, unless
> the FDW signals that it can enforce the constraint.

I think that would be missing the point. If a constraint is NOT
ENFORCED, then the query planner presumably won't rely on it for
planning purposes, but the whole point of having constraints on
foreign tables is that we want the query planner to do just that.

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

On ons, 2012-03-14 at 17:16 -0400, Robert Haas wrote:
> If a constraint is NOT ENFORCED, then the query planner presumably
> won't rely on it for planning purposes

Why do you presume that?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On ons, 2012-03-14 at 17:16 -0400, Robert Haas wrote:
>> If a constraint is NOT ENFORCED, then the query planner presumably
>> won't rely on it for planning purposes

> Why do you presume that?

What does SQL:2011 say exactly about the semantics of NOT ENFORCED?
Is an implementation allowed to fail in undefined ways if a constraint
is marked NOT ENFORCED and is not actually true?

regards, tom lane

On Wed, Mar 14, 2012 at 5:21 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On ons, 2012-03-14 at 17:16 -0400, Robert Haas wrote:
>> If a constraint is NOT ENFORCED, then the query planner presumably
>> won't rely on it for planning purposes
>
> Why do you presume that?

Well, as Tom alludes to, I'm guessing that NOT ENFORCED is not a
license to deliver wrong answers. But also as Tom says, what does the
spec say?

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

On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> >> I think that instead of inventing new grammar productions and a new
> >> node type for this, you should just reuse the existing productions for
> >> LIKE clauses and then reject invalid options during parse analysis.
> >
> > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> > submit that as a separate patch?
>
> I don't see any reason to do that. I merely meant that you could
> reuse TableLikeClause or maybe even TableElement in the grammer for
> CreateForeignTableStmt.

Next WIP patch attached implementing this via reusing TableLikeClause
and refactoring transformTableLikeClause().

What say?

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

(2012/03/15 0:29), Tom Lane wrote:
> The posted patch for file_fdw takes the
> approach of silently filtering out rows for which they're not true,
> which is not obviously the right thing either --- quite aside from
> whether that's a sane semantics, it's not going to scale to foreign key
> constraints, and even for simple NOT NULL and CHECK constraints it
> results in a runtime penalty on selects, which is not what people would
> expect from a constraint.

I investigated DB2 a little bit. In DB2, the user can specify the
VALIDATE_DATA_FILE option as a generic option for an external table
attached to a data file, which specifies if the wrapper verifies that
the data file is sorted. How about introducing this kind of option to
file_fdw? It might be better that the default value for the option is
'false', and if the value is set to 'true', then file_fdw verifies NOT
NULL, CHECK, and foreign key constraints.

Best regards,
Etsuro Fujita

Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
> On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > >> I think that instead of inventing new grammar productions and a new
> > >> node type for this, you should just reuse the existing productions for
> > >> LIKE clauses and then reject invalid options during parse analysis.
> > >
> > > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> > > submit that as a separate patch?
> >
> > I don't see any reason to do that. I merely meant that you could
> > reuse TableLikeClause or maybe even TableElement in the grammer for
> > CreateForeignTableStmt.
>
> Next WIP patch attached implementing this via reusing TableLikeClause
> and refactoring transformTableLikeClause().
>
> What say?

Looks much better to me, but the use of strcmp() doesn't look good.
ISTM that stmtType is mostly used for error messages. I think you
should add some kind of identifier (such as the original parser Node)
into the CreateStmtContext so that you can do a IsA() test instead -- a
bit more invasive as a patch, but much cleaner.

Also the error messages need more work.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Mar 15, 2012 at 11:23:43AM -0300, Alvaro Herrera wrote:
> Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
> > On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> > > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > > >> I think that instead of inventing new grammar productions and
> > > >> a new node type for this, you should just reuse the existing
> > > >> productions for LIKE clauses and then reject invalid options
> > > >> during parse analysis.
> > > >
> > > > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE
> > > > TABLE and submit that as a separate patch?
> > >
> > > I don't see any reason to do that. I merely meant that you
> > > could reuse TableLikeClause or maybe even TableElement in the
> > > grammer for CreateForeignTableStmt.
> >
> > Next WIP patch attached implementing this via reusing
> > TableLikeClause and refactoring transformTableLikeClause().
> >
> > What say?
>
> Looks much better to me, but the use of strcmp() doesn't look good.
> ISTM that stmtType is mostly used for error messages. I think you
> should add some kind of identifier (such as the original parser
> Node) into the CreateStmtContext so that you can do a IsA() test
> instead -- a bit more invasive as a patch, but much cleaner.

OK

> Also the error messages need more work.

What sort?

The more I look at this, the more I think that CREATE TABLE and CREATE
FOREIGN TABLE should be merged, but that's the subject of a later
patch.

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

On Thu, Mar 15, 2012 at 10:23 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Looks much better to me, but the use of strcmp() doesn't look good.
> ISTM that stmtType is mostly used for error messages.  I think you
> should add some kind of identifier (such as the original parser Node)
> into the CreateStmtContext so that you can do a IsA() test instead -- a
> bit more invasive as a patch, but much cleaner.

+1. Or maybe add a relkind to CreateStmt, if it isn't there already,
and test that.

> Also the error messages need more work.

+1. I suggest something like "ERROR: foreign tables do not support
LIKE INCLUDING INDEXES".

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

On ons, 2012-03-14 at 17:38 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On ons, 2012-03-14 at 17:16 -0400, Robert Haas wrote:
> >> If a constraint is NOT ENFORCED, then the query planner presumably
> >> won't rely on it for planning purposes
>
> > Why do you presume that?
>
> What does SQL:2011 say exactly about the semantics of NOT ENFORCED?
> Is an implementation allowed to fail in undefined ways if a constraint
> is marked NOT ENFORCED and is not actually true?

It doesn't say anything about that. I might have to dig deeper into the
change proposals to see if any rationale came with this change.

But in any case, even if we spell it differently, I think there are use
cases for a constraint mode that says, assume it's true for optimization
purposes, but don't spend any cycles on verifying it. And that
constraint mode could apply to regular tables and foreign tables alike.

On Thu, Mar 15, 2012 at 11:23:43AM -0300, Alvaro Herrera wrote:
> Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
> > On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> > > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > > >> I think that instead of inventing new grammar productions and a new
> > > >> node type for this, you should just reuse the existing productions for
> > > >> LIKE clauses and then reject invalid options during parse analysis.
> > > >
> > > > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> > > > submit that as a separate patch?
> > >
> > > I don't see any reason to do that. I merely meant that you could
> > > reuse TableLikeClause or maybe even TableElement in the grammer for
> > > CreateForeignTableStmt.
> >
> > Next WIP patch attached implementing this via reusing TableLikeClause
> > and refactoring transformTableLikeClause().
> >
> > What say?
>
> Looks much better to me, but the use of strcmp() doesn't look good.
> ISTM that stmtType is mostly used for error messages.

Is it used for anything at all?

> I think you should add some kind of identifier (such as the original
> parser Node) into the CreateStmtContext so that you can do a IsA()
> test instead -- a bit more invasive as a patch, but much cleaner.

OK

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

On Thu, Mar 15, 2012 at 11:23:43AM -0300, Alvaro Herrera wrote:
> Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
> > On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> > > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > > >> I think that instead of inventing new grammar productions and a new
> > > >> node type for this, you should just reuse the existing productions for
> > > >> LIKE clauses and then reject invalid options during parse analysis.
> > > >
> > > > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> > > > submit that as a separate patch?
> > >
> > > I don't see any reason to do that. I merely meant that you could
> > > reuse TableLikeClause or maybe even TableElement in the grammer for
> > > CreateForeignTableStmt.
> >
> > Next WIP patch attached implementing this via reusing TableLikeClause
> > and refactoring transformTableLikeClause().
> >
> > What say?
>
> Looks much better to me, but the use of strcmp() doesn't look good.
> ISTM that stmtType is mostly used for error messages. I think you
> should add some kind of identifier (such as the original parser Node)
> into the CreateStmtContext so that you can do a IsA() test instead -- a
> bit more invasive as a patch, but much cleaner.
>
> Also the error messages need more work.

How about this 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

On Fri, Mar 23, 2012 at 11:38:56AM -0700, David Fetter wrote:
> On Thu, Mar 15, 2012 at 11:23:43AM -0300, Alvaro Herrera wrote:
> > Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
> > > On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
> > > > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
> > > > >> I think that instead of inventing new grammar productions and a new
> > > > >> node type for this, you should just reuse the existing productions for
> > > > >> LIKE clauses and then reject invalid options during parse analysis.
> > > > >
> > > > > OK.  Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
> > > > > submit that as a separate patch?
> > > >
> > > > I don't see any reason to do that. I merely meant that you could
> > > > reuse TableLikeClause or maybe even TableElement in the grammer for
> > > > CreateForeignTableStmt.
> > >
> > > Next WIP patch attached implementing this via reusing TableLikeClause
> > > and refactoring transformTableLikeClause().
> > >
> > > What say?
> >
> > Looks much better to me, but the use of strcmp() doesn't look good.
> > ISTM that stmtType is mostly used for error messages. I think you
> > should add some kind of identifier (such as the original parser Node)
> > into the CreateStmtContext so that you can do a IsA() test instead -- a
> > bit more invasive as a patch, but much cleaner.
> >
> > Also the error messages need more work.
>
> How about this one?

Oops, forgot to put the latest docs in.

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

On 23 March 2012 19:07, David Fetter <david(at)fetter(dot)org> wrote:
> On Fri, Mar 23, 2012 at 11:38:56AM -0700, David Fetter wrote:
>> How about this one?
>
> Oops, forgot to put the latest docs in.

I think the docs need some additional supporting content. The LIKE
clause and its source_table parameter isn't explained on the CREATE
FOREIGN TABLE page. There's no mention of the like_option parameter
too which should be valid since you can specify whether it includes
comments (among less relevant options).

Also you appear to have modified the documented command definition so
that OPTIONS can't be applied per-column anymore. It's now placed "[,
... ]" prior to the column's OPTIONS clause.

The patch works for me though, and allows tables, foreign tables,
views and composite types to be used in the LIKE clause.

Thom

The patch uses literals such as 'r' to identify the relkind values.
This should be using RELKIND_RELATION et al instead -- see
src/include/catalog/pg_class.h.

Other than that, it seems simple enough ...

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 23 March 2012 18:38, David Fetter <david(at)fetter(dot)org> wrote:
> On Thu, Mar 15, 2012 at 11:23:43AM -0300, Alvaro Herrera wrote:
>> Excerpts from David Fetter's message of jue mar 15 02:28:28 -0300 2012:
>> > On Wed, Mar 14, 2012 at 12:06:20PM -0400, Robert Haas wrote:
>> > > On Wed, Mar 14, 2012 at 10:22 AM, David Fetter <david(at)fetter(dot)org> wrote:
>> > > >> I think that instead of inventing new grammar productions and a new
>> > > >> node type for this, you should just reuse the existing productions for
>> > > >> LIKE clauses and then reject invalid options during parse analysis.
>> > > >
>> > > > OK. Should I first merge CREATE FOREIGN TABLE with CREATE TABLE and
>> > > > submit that as a separate patch?
>> > >
>> > > I don't see any reason to do that. I merely meant that you could
>> > > reuse TableLikeClause or maybe even TableElement in the grammer for
>> > > CreateForeignTableStmt.
>> >
>> > Next WIP patch attached implementing this via reusing TableLikeClause
>> > and refactoring transformTableLikeClause().
>> >
>> > What say?
>>
>> Looks much better to me, but the use of strcmp() doesn't look good.
>> ISTM that stmtType is mostly used for error messages. I think you
>> should add some kind of identifier (such as the original parser Node)
>> into the CreateStmtContext so that you can do a IsA() test instead -- a
>> bit more invasive as a patch, but much cleaner.
>>
>> Also the error messages need more work.
>
> How about this one?
>

I spotted a couple of other issues during testing:

* You're still allowing INCLUDING DEFAULTS and INCLUDING STORAGE, even
though these options are not supported on foreign tables.

* If I do INCLUDING ALL, I get an error because of the unsupported
options. I think that "ALL" in this context needs to be made to mean
all the options that foreign tables support (just COMMENTS at the
moment).

Regards,
Dean

Excerpts from Dean Rasheed's message of sáb jun 23 18:08:31 -0400 2012:

> I spotted a couple of other issues during testing:

David, when you generate a new version of the patch, please also make
sure to use RELKIND_RELATION and RELKIND_FOREIGN instead of 'r' and 'f'.

> * You're still allowing INCLUDING DEFAULTS and INCLUDING STORAGE, even
> though these options are not supported on foreign tables.

Maybe the code should list options allowed instead of the ones
disallowed.

> * If I do INCLUDING ALL, I get an error because of the unsupported
> options. I think that "ALL" in this context needs to be made to mean
> all the options that foreign tables support (just COMMENTS at the
> moment).

I agree.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 24 June 2012 04:01, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>
> Excerpts from Dean Rasheed's message of sáb jun 23 18:08:31 -0400 2012:
>
>> I spotted a couple of other issues during testing:
>
> David, when you generate a new version of the patch, please also make
> sure to use RELKIND_RELATION and RELKIND_FOREIGN instead of 'r' and 'f'.
>
>> * You're still allowing INCLUDING DEFAULTS and INCLUDING STORAGE, even
>> though these options are not supported on foreign tables.
>
> Maybe the code should list options allowed instead of the ones
> disallowed.
>
>> * If I do INCLUDING ALL, I get an error because of the unsupported
>> options. I think that "ALL" in this context needs to be made to mean
>> all the options that foreign tables support (just COMMENTS at the
>> moment).
>
> I agree.
>

David, do you have an updated version of this patch?

Regards,
Dean

On lör, 2012-06-23 at 23:08 +0100, Dean Rasheed wrote:
> I spotted a couple of other issues during testing:
>
> * You're still allowing INCLUDING DEFAULTS and INCLUDING STORAGE, even
> though these options are not supported on foreign tables.
>
> * If I do INCLUDING ALL, I get an error because of the unsupported
> options. I think that "ALL" in this context needs to be made to mean
> all the options that foreign tables support (just COMMENTS at the
> moment).

Note that when I added CREATE TABLE LIKE to support composite types, it
was decided to ignore non-applicable options (like copying indexes from
types or views etc.). The same should be done here, unless we have
reasons to revise the earlier decision.