Re: inherit support for foreign tables

Hi hackers,

I'd like to propose adding inheritance support for foriegn tables.
David Fetter mentioned this feature last July, but it seems stalled.

http://www.postgresql.org/message-id/20130719005601.GA5760@fetter.org

Supporting inheritance by foreign tables allows us to distribute query
to remote servers by using foreign tables as partition table of a
(perhaps ordinary) table. For this purpose, I think that constraint
exclusion is necessary.

As result of extending Devid's patch for PoC, and AFAIS we need these changes:

1) Add INHERITS(rel, ...) clause to CREATE/ALTER FOREIGN TABLE
Apperantly we need to add new syntax to define parent table(s) of a
foreign table. We have options about the position of INHERIT clause,
but I'd prefer before SERVER clause because having options specific to
foreign tables at the tail would be most extensible.

a) CREATE FOREIGN TABLE child (...) INHERITS(p1, p2) SERVER server;
b) CREATE FOREIGN TABLE child (...) SERVER server INHERITS(p1, p2);

2) Allow foreign tables to have CHECK constraints
Like NOT NULL, I think we don't need to enforce the check duroing
INSERT/UPDATE against foreign table.

3) Allow foreign table as a child node of Append
Currently prepunion.c assumes that children of Append have
RELKIND_RELATION as relkind always, so we need to set relkind of child
RTE explicitly.

Please see attached PoC patch. I'll enhance implementation, tests and
document and submit the patch for the next CF.

Regards,
--
Shigeru HANADA

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> I'd like to propose adding inheritance support for foriegn tables.
> David Fetter mentioned this feature last July, but it seems stalled.
> http://www.postgresql.org/message-id/20130719005601.GA5760@fetter.org

The discussion there pointed out that not enough consideration had been
given to interactions with other commands. I'm not really satisfied
with your analysis here. In particular:

> 2) Allow foreign tables to have CHECK constraints
> Like NOT NULL, I think we don't need to enforce the check duroing
> INSERT/UPDATE against foreign table.

Really? It's one thing to say that somebody who adds a CHECK constraint
to a foreign table is responsible to make sure that the foreign data will
satisfy the constraint. It feels like a different thing to say that ALTER
TABLE ADD CONSTRAINT applied to a parent table will silently assume that
some child table that happens to be foreign doesn't need any enforcement.

Perhaps more to the point, inheritance trees are the main place where the
planner depends on the assumption that CHECK constraints represent
reality. Are we really prepared to say that it's the user's fault if the
planner generates an incorrect plan on the strength of a CHECK constraint
that's not actually satisfied by the foreign data? If so, that had better
be documented by this patch. But for a project that refuses to let people
create a local CHECK or FOREIGN KEY constraint without mechanically
checking it, it seems pretty darn weird to be so laissez-faire about
constraints on foreign data.

regards, tom lane

On Thu, Nov 14, 2013 at 12:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 2) Allow foreign tables to have CHECK constraints
>> Like NOT NULL, I think we don't need to enforce the check duroing
>> INSERT/UPDATE against foreign table.
>
> Really? It's one thing to say that somebody who adds a CHECK constraint
> to a foreign table is responsible to make sure that the foreign data will
> satisfy the constraint. It feels like a different thing to say that ALTER
> TABLE ADD CONSTRAINT applied to a parent table will silently assume that
> some child table that happens to be foreign doesn't need any enforcement.
>
> Perhaps more to the point, inheritance trees are the main place where the
> planner depends on the assumption that CHECK constraints represent
> reality. Are we really prepared to say that it's the user's fault if the
> planner generates an incorrect plan on the strength of a CHECK constraint
> that's not actually satisfied by the foreign data? If so, that had better
> be documented by this patch. But for a project that refuses to let people
> create a local CHECK or FOREIGN KEY constraint without mechanically
> checking it, it seems pretty darn weird to be so laissez-faire about
> constraints on foreign data.

I can see both sides of this issue. We certainly have no way to force
the remote side to enforce CHECK constraints defined on the local
side, because the remote side could also be accepting writes from
other sources that don't have any matching constraint. But having said
that, I can't see any particularly principled reason why we shouldn't
at least check the new rows we insert ourselves. After all, we could
be in the situation proposed by KaiGai Kohei, where the foreign data
wrapper API is being used as a surrogate storage engine API - i.e.
there are no writers to the foreign side except ourselves. In that
situation, it would seem odd to randomly fail to enforce the
constraints.

On the other hand, the performance costs of checking every row bound
for the remote table could be quite steep. Consider an update on an
inheritance hierarchy that sets a = a + 1 for every row. If we don't
worry about verifying that the resulting rows satisfy all local-side
constraints, we can potentially ship a single update statement to the
remote server and let it do all the work there. But if we DO have to
worry about that, then we're going to have to ship every updated row
over the wire in at least one direction, if not both. If the purpose
of adding CHECK constraints was to enable constraint exclusion, that's
a mighty steep price to pay for it.

I think it's been previously proposed that we have some version of a
CHECK constraint that effectively acts as an assertion for query
optimization purposes, but isn't actually enforced by the system. I
can see that being useful in a variety of situations, including this
one.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Nov 14, 2013 at 12:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 2) Allow foreign tables to have CHECK constraints
>>> Like NOT NULL, I think we don't need to enforce the check duroing
>>> INSERT/UPDATE against foreign table.

>> Really?

> I think it's been previously proposed that we have some version of a
> CHECK constraint that effectively acts as an assertion for query
> optimization purposes, but isn't actually enforced by the system. I
> can see that being useful in a variety of situations, including this
> one.

Yeah, I think it would be much smarter to provide a different syntax
to explicitly represent the notion that we're only assuming the condition
is true, and not trying to enforce it.

regards, tom lane

2013/11/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think it's been previously proposed that we have some version of a
>> CHECK constraint that effectively acts as an assertion for query
>> optimization purposes, but isn't actually enforced by the system. I
>> can see that being useful in a variety of situations, including this
>> one.
>
> Yeah, I think it would be much smarter to provide a different syntax
> to explicitly represent the notion that we're only assuming the condition
> is true, and not trying to enforce it.

I'd like to revisit this feature.

Explicit notation to represent not-enforcing (assertive?) is an
interesting idea. I'm not sure which word is appropriate, but for
example, let's use the word ASSERTIVE as a new constraint attribute.

CREATE TABLE parent (
id int NOT NULL,
group int NOT NULL,
name text
);

CREATE FOREIGN TABLE child_grp1 (
/* no additional column */
) INHERITS (parent) SERVER server1;
ALTER TABLE child_grp1 ADD CONSTRAINT chk_group1 CHECK (group = 1) ASSERTIVE;

If ASSERTIVE is specified, it's not guaranteed that the constraint is
enforced completely, so it should be treated as a hint for planner.
As Robert mentioned, enforcing as much as we can during INSERT/UPDATE
is one option about this issue.

In addition, an idea which I can't throw away is to assume that all
constraints defined on foreign tables as ASSERTIVE. Foreign tables
potentially have dangers to have "wrong" data by updating source data
not through foreign tables. This is not specific to an FDW, so IMO
constraints defined on foreign tables are basically ASSERTIVE. Of
course PG can try to maintain data correct, but always somebody might
break it.

Besides CHECK constraints, currently NOT NULL constraints are
virtually ASSERTIVE (not enforcing). Should it also be noted
explicitly?

Thoughts?

--
Shigeru HANADA

From PostgreSQL manual:
"A serious limitation of the inheritance feature is that indexes
(including unique constraints) and foreign key constraints only apply to
single tables, not to their inheritance children."

But is it possible to use index for derived table at all?
Why sequential search is used for derived table in the example below:

create table base_table (x integer primary key);
create table derived_table (y integer) inherits (base_table);
insert into base_table values (1);
insert into derived_table values (2,2);
create index derived_index on derived_table(x);
explain select * from base_table where x>=0;
QUERY PLAN
----------------------------------------------------------------------------------------------
Append (cost=0.14..4.56 rows=81 width=4)
-> Index Only Scan using base_table_pkey on base_table
(cost=0.14..3.55 rows=80 width=4)
Index Cond: (x >= 0)
-> Seq Scan on derived_table (cost=0.00..1.01 rows=1 width=4)
Filter: (x >= 0)
(5 rows)

On Tue, Jan 14, 2014 at 12:07 PM, knizhnik <knizhnik(at)garret(dot)ru> wrote:
> But is it possible to use index for derived table at all?

Yes, the planner will do an index scan when it makes sense.

> Why sequential search is used for derived table in the example below:

> insert into derived_table values (2,2);
> create index derived_index on derived_table(x);
> explain select * from base_table where x>=0;

With only 1 row in the table, the planner decides there's no point in
scanning the index. Try with more realistic data.

Regards,
Marti

Hi all,

2014/1/14 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> I'd like to revisit this feature.

Attached patch allows a foreign table to be a child of a table. It
also allows foreign tables to have CHECK constraints. These changes
provide us a chance to propagate query load to multiple servers via
constraint exclusion. If FDW supports async operation against remote
server, parallel processing (not stable but read-only case would be
find) can be achieved, though overhead of FDW mechanism is still
there.

Though this would be debatable, in current implementation, constraints
defined on a foreign table (now only NOT NULL and CHECK are supported)
are not enforced during INSERT or UPDATE executed against foreign
tables. This means that retrieved data might violates the constraints
defined on local side. This is debatable issue because integrity of
data is important for DBMS, but in the first cut, it is just
documented as a note.

Because I don't see practical case to have a foreign table as a
parent, and it avoid a problem about recursive ALTER TABLE operation,
foreign tables can't be a parent. An example of such problem is
adding constraint which is not unsupported for foreign tables to the
parent of foreign table. Propagated operation can be applied to
ordinary tables in the inheritance tree, but can't be to foreign
tables. If we allow foreign tables to be parent, it's difficult to
process ordinary tables below foreign tables in current traffic cop
mechanism.

For other commands recursively processed such as ANALYZE, foreign
tables in the leaf of inheritance tree are ignored.

Any comments or questions are welcome.
--
Shigeru HANADA

On 11/18/13, 8:36 AM, Robert Haas wrote:
> On the other hand, the performance costs of checking every row bound
> for the remote table could be quite steep. Consider an update on an
> inheritance hierarchy that sets a = a + 1 for every row. If we don't
> worry about verifying that the resulting rows satisfy all local-side
> constraints, we can potentially ship a single update statement to the
> remote server and let it do all the work there. But if we DO have to
> worry about that, then we're going to have to ship every updated row
> over the wire in at least one direction, if not both. If the purpose
> of adding CHECK constraints was to enable constraint exclusion, that's
> a mighty steep price to pay for it.

A sophisticated enough FDW could verify that the appropriate check already existed in tho foreign side, or it could do something like:

BEGIN;
UPDATE SET ... WHERE <where>
SELECT EXISTS( SELECT 1 WHERE <where> AND NOT (<check condition>) );

And then rollback if the SELECT returns true.

But obviously you can't always do that, so I think there's a place for both true constraints and "suggested constraints".
--
Jim C. Nasby, Data Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

(2014/01/14 18:24), Shigeru Hanada wrote:
> 2013/11/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> I think it's been previously proposed that we have some version of a
>>> CHECK constraint that effectively acts as an assertion for query
>>> optimization purposes, but isn't actually enforced by the system. I
>>> can see that being useful in a variety of situations, including this
>>> one.
>>
>> Yeah, I think it would be much smarter to provide a different syntax
>> to explicitly represent the notion that we're only assuming the condition
>> is true, and not trying to enforce it.
>
> I'd like to revisit this feature.
>
> Explicit notation to represent not-enforcing (assertive?) is an
> interesting idea. I'm not sure which word is appropriate, but for
> example, let's use the word ASSERTIVE as a new constraint attribute.
>
> CREATE TABLE parent (
> id int NOT NULL,
> group int NOT NULL,
> name text
> );
>
> CREATE FOREIGN TABLE child_grp1 (
> /* no additional column */
> ) INHERITS (parent) SERVER server1;
> ALTER TABLE child_grp1 ADD CONSTRAINT chk_group1 CHECK (group = 1) ASSERTIVE;
>
> If ASSERTIVE is specified, it's not guaranteed that the constraint is
> enforced completely, so it should be treated as a hint for planner.
> As Robert mentioned, enforcing as much as we can during INSERT/UPDATE
> is one option about this issue.
>
> In addition, an idea which I can't throw away is to assume that all
> constraints defined on foreign tables as ASSERTIVE. Foreign tables
> potentially have dangers to have "wrong" data by updating source data
> not through foreign tables. This is not specific to an FDW, so IMO
> constraints defined on foreign tables are basically ASSERTIVE. Of
> course PG can try to maintain data correct, but always somebody might
> break it.
> qu
>
Does it make sense to apply "assertive" CHECK constraint on the qual
of ForeignScan to filter out tuples with violated values at the local
side, as if row-level security feature doing.
It enables to handle a situation that planner expects only "clean"
tuples are returned but FDW driver is unavailable to anomalies.

Probably, this additional check can be turned on/off on the fly,
if FDW driver has a way to inform the core system its capability,
like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip
local checks.

> Besides CHECK constraints, currently NOT NULL constraints are
> virtually ASSERTIVE (not enforcing). Should it also be noted
> explicitly?
>
Backward compatibility....

NOT NULL [ASSERTIVE] might be an option.

Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Thanks for the comments.

2014/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> In addition, an idea which I can't throw away is to assume that all
>> constraints defined on foreign tables as ASSERTIVE. Foreign tables
>> potentially have dangers to have "wrong" data by updating source data
>> not through foreign tables. This is not specific to an FDW, so IMO
>> constraints defined on foreign tables are basically ASSERTIVE. Of
>> course PG can try to maintain data correct, but always somebody might
>> break it.
>> qu
>>
> Does it make sense to apply "assertive" CHECK constraint on the qual
> of ForeignScan to filter out tuples with violated values at the local
> side, as if row-level security feature doing.
> It enables to handle a situation that planner expects only "clean"
> tuples are returned but FDW driver is unavailable to anomalies.
>
> Probably, this additional check can be turned on/off on the fly,
> if FDW driver has a way to inform the core system its capability,
> like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip
> local checks.

Hmm, IIUC you mean that local users can't (or don't need to) know that
data which violates the local constraints exist on remote side.
Applying constraints to the data which is modified through FDW would
be necessary as well. In that design, FDW is a bidirectional filter
which provides these features:

1) Don't push wrong data into remote data source, by applying local
constraints to the result of the modifying query executed on local PG.
This is not perfect filter, because remote constraints don't mapped
automatically or perfectly (imagine constraints which is available on
remote but is not supported in PG).
2) Don't retrieve wrong data from remote to local PG, by applying
local constraints

I have a concern about consistency. It has not been supported, but
let's think of Aggregate push-down invoked by a query below.

SELECT count(*) FROM remote_table;

If this query was fully pushed down, the result is the # of records
exist on remote side, but the result would be # of valid records when
we don't push down the aggregate. This would confuse users.

>> Besides CHECK constraints, currently NOT NULL constraints are
>> virtually ASSERTIVE (not enforcing). Should it also be noted
>> explicitly?
>>
> Backward compatibility….

Yep, backward compatibility (especially visible ones to users) should
be minimal, ideally zero.

> NOT NULL [ASSERTIVE] might be an option.

Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow
ingASSERTIVE for only foreign tables? It makes sense, though we need
consider exclusiveness . But It needs to default to ASSERTIVE on
foreign tables, and NOT ASSERTIVE (means "forced") on others. Isn't
is too complicated?

CREATE FOREIGN TABLE foo (
id int NOT NULL ASSERTIVE CHECK (id > 1) ASSERTIVE,
…
CONSTRAINT chk_foo_name_upper CHECK (upper(name) = name) ASSERTIVE
) SERVER server;

BTW, I noticed that this is like push-down-able expressions in
JOIN/WHERE. We need to check a CHECK constraint defined on a foreign
tables contains only expressions which have same semantics as remote
side (in practice, built-in and immutable)?
--
Shigeru HANADA

(2014/01/21 11:44), Shigeru Hanada wrote:
> Thanks for the comments.
>
> 2014/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> In addition, an idea which I can't throw away is to assume that all
>>> constraints defined on foreign tables as ASSERTIVE. Foreign tables
>>> potentially have dangers to have "wrong" data by updating source data
>>> not through foreign tables. This is not specific to an FDW, so IMO
>>> constraints defined on foreign tables are basically ASSERTIVE. Of
>>> course PG can try to maintain data correct, but always somebody might
>>> break it.
>>> qu
>>>
>> Does it make sense to apply "assertive" CHECK constraint on the qual
>> of ForeignScan to filter out tuples with violated values at the local
>> side, as if row-level security feature doing.
>> It enables to handle a situation that planner expects only "clean"
>> tuples are returned but FDW driver is unavailable to anomalies.
>>
>> Probably, this additional check can be turned on/off on the fly,
>> if FDW driver has a way to inform the core system its capability,
>> like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip
>> local checks.
>
> Hmm, IIUC you mean that local users can't (or don't need to) know that
> data which violates the local constraints exist on remote side.
> Applying constraints to the data which is modified through FDW would
> be necessary as well. In that design, FDW is a bidirectional filter
> which provides these features:
>
> 1) Don't push wrong data into remote data source, by applying local
> constraints to the result of the modifying query executed on local PG.
> This is not perfect filter, because remote constraints don't mapped
> automatically or perfectly (imagine constraints which is available on
> remote but is not supported in PG).
> 2) Don't retrieve wrong data from remote to local PG, by applying
> local constraints
>
Yes. (1) can be done with ExecConstraints prior to FDW callback on
UPDATE or INSERT, even not a perfect solution because of side-channel
on the remote data source. For (2), my proposition tries to drop
retrieved violated tuples, however, the result is same.

> I have a concern about consistency. It has not been supported, but
> let's think of Aggregate push-down invoked by a query below.
>
> SELECT count(*) FROM remote_table;
>
> If this query was fully pushed down, the result is the # of records
> exist on remote side, but the result would be # of valid records when
> we don't push down the aggregate. This would confuse users.
>
Hmm. In this case, FDW driver needs to be responsible to push-down
the additional check quals into remote side, so it does not work
transparently towards the ForeignScan.
It might be a little bit complicated suggestion for the beginning
of the efforts.

>>> Besides CHECK constraints, currently NOT NULL constraints are
>>> virtually ASSERTIVE (not enforcing). Should it also be noted
>>> explicitly?
>>>
>> Backward compatibility….
>
> Yep, backward compatibility (especially visible ones to users) should
> be minimal, ideally zero.
>
>> NOT NULL [ASSERTIVE] might be an option.
>
> Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow
> ingASSERTIVE for only foreign tables? It makes sense, though we need
> consider exclusiveness . But It needs to default to ASSERTIVE on
> foreign tables, and NOT ASSERTIVE (means "forced") on others. Isn't
> is too complicated?
>
I think it is not easy to implement assertive checks, except for
foreign tables, because all the write stuff to regular tables are
managed by PostgreSQL itself.
So, it is a good first step to add support "ASSERTIVE" CHECK on
foreign table only, and to enforce FDW drivers nothing special
from my personal sense.

How about committer's opinion?

Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Mon, Jan 20, 2014 at 9:44 PM, Shigeru Hanada
<shigeru(dot)hanada(at)gmail(dot)com> wrote:
> Thanks for the comments.
>
> 2014/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> In addition, an idea which I can't throw away is to assume that all
>>> constraints defined on foreign tables as ASSERTIVE. Foreign tables
>>> potentially have dangers to have "wrong" data by updating source data
>>> not through foreign tables. This is not specific to an FDW, so IMO
>>> constraints defined on foreign tables are basically ASSERTIVE. Of
>>> course PG can try to maintain data correct, but always somebody might
>>> break it.
>>> qu
>>>
>> Does it make sense to apply "assertive" CHECK constraint on the qual
>> of ForeignScan to filter out tuples with violated values at the local
>> side, as if row-level security feature doing.
>> It enables to handle a situation that planner expects only "clean"
>> tuples are returned but FDW driver is unavailable to anomalies.
>>
>> Probably, this additional check can be turned on/off on the fly,
>> if FDW driver has a way to inform the core system its capability,
>> like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip
>> local checks.
>
> Hmm, IIUC you mean that local users can't (or don't need to) know that
> data which violates the local constraints exist on remote side.
> Applying constraints to the data which is modified through FDW would
> be necessary as well. In that design, FDW is a bidirectional filter
> which provides these features:
>
> 1) Don't push wrong data into remote data source, by applying local
> constraints to the result of the modifying query executed on local PG.
> This is not perfect filter, because remote constraints don't mapped
> automatically or perfectly (imagine constraints which is available on
> remote but is not supported in PG).
> 2) Don't retrieve wrong data from remote to local PG, by applying
> local constraints
>
> I have a concern about consistency. It has not been supported, but
> let's think of Aggregate push-down invoked by a query below.
>
> SELECT count(*) FROM remote_table;
>
> If this query was fully pushed down, the result is the # of records
> exist on remote side, but the result would be # of valid records when
> we don't push down the aggregate. This would confuse users.
>
>>> Besides CHECK constraints, currently NOT NULL constraints are
>>> virtually ASSERTIVE (not enforcing). Should it also be noted
>>> explicitly?
>>>
>> Backward compatibility….
>
> Yep, backward compatibility (especially visible ones to users) should
> be minimal, ideally zero.
>
>> NOT NULL [ASSERTIVE] might be an option.
>
> Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow
> ingASSERTIVE for only foreign tables? It makes sense, though we need
> consider exclusiveness . But It needs to default to ASSERTIVE on
> foreign tables, and NOT ASSERTIVE (means "forced") on others. Isn't
> is too complicated?
>
> CREATE FOREIGN TABLE foo (
> id int NOT NULL ASSERTIVE CHECK (id > 1) ASSERTIVE,
> …
> CONSTRAINT chk_foo_name_upper CHECK (upper(name) = name) ASSERTIVE
> ) SERVER server;
>
> BTW, I noticed that this is like push-down-able expressions in
> JOIN/WHERE. We need to check a CHECK constraint defined on a foreign
> tables contains only expressions which have same semantics as remote
> side (in practice, built-in and immutable)?

I don't think that that ASSERTIVE is going to fly, because "assertive"
means (sayeth the Google) "having or showing a confident and forceful
personality", which is not what we mean here. It's tempting to do
something like try to replace the keyword "check" with "assume" or
"assert" or (stretching) "assertion", but that would require whichever
one we picked to be a fully-reserved keyword, which I can't think is
going to get much support here, for entirely understandable reasons.
So I think we should look for another option.

Currently, constraints can be marked NO INHERIT (though this seems to
have not been fully documented, as the ALTER TABLE page doesn't
mention it anywhere) or NOT VALID, so I'm thinking maybe we should go
with something along those lines. Some ideas:

- NO CHECK. The idea of writing CHECK (id > 1) NO CHECK is pretty
hilarious, though.
- NO VALIDATE. But then people need to understand that NOT VALID
means "we didn't validate it yet" while "no validate" means "we don't
ever intend to validate it", which could be confusing.
- NO ENFORCE. Requires a new (probably unreserved) keyword.
- NOT VALIDATED or NOT CHECKED. Same problems as NO CHECK and NO
VALIDATE, respectively, plus now we have to create a new keyword.

Another idea is to apply an extensible-options syntax to constraints,
like we do for EXPLAIN, VACUUM, etc. Like maybe:

CHECK (id > 1) OPTIONS (enforced false, valid true)

Yet another idea is to consider validity a three-state property:
either the constraint is valid (because we have checked it and are
enforcing it), or it is not valid (because we are enforcing it but
have not checked the pre-existing data), or it is assumed true
(because we are not checking or enforcing it but are believing it
anyway). So then we could have a syntax like this:

CHECK (id > 1) VALIDATE { ON | OFF | ASSERTION }

Other ideas?

One thing that's bugging me a bit about this whole line of attack is
that, in the first instance, the whole goal here is to support
inheritance hierarchies that mix ordinary tables with foreign tables.
If you have a table with children some of which are inherited and
others of which are not inherited, you're very likely going to want
your constraints enforced for real on the children that are tables and
assumed true on the children that are foreign tables, and none of what
we're talking about here gets us to that, because we normally want the
constraints to be identical throughout the inheritance hierarchy.
Maybe there's some way around that, but I'm back to wondering if it
wouldn't be better to simply silently force any constraints on a
foreign-table into assertion mode. That could be done without any new
syntax at all, and frankly I think it's what people are going to want
more often than not.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> One thing that's bugging me a bit about this whole line of attack is
> that, in the first instance, the whole goal here is to support
> inheritance hierarchies that mix ordinary tables with foreign tables.
> If you have a table with children some of which are inherited and
> others of which are not inherited, you're very likely going to want
> your constraints enforced for real on the children that are tables and
> assumed true on the children that are foreign tables, and none of what
> we're talking about here gets us to that, because we normally want the
> constraints to be identical throughout the inheritance hierarchy.

There's a nearby thread that's addressing this same question, in which
I make the case (again) that the right thing for postgres_fdw constraints
is that they're just assumed true. So I'm not sure why this conversation
is proposing to implement a lot of mechanism to do something different
from that.

regards, tom lane

On Tue, Jan 21, 2014 at 3:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> One thing that's bugging me a bit about this whole line of attack is
>> that, in the first instance, the whole goal here is to support
>> inheritance hierarchies that mix ordinary tables with foreign tables.
>> If you have a table with children some of which are inherited and
>> others of which are not inherited, you're very likely going to want
>> your constraints enforced for real on the children that are tables and
>> assumed true on the children that are foreign tables, and none of what
>> we're talking about here gets us to that, because we normally want the
>> constraints to be identical throughout the inheritance hierarchy.
>
> There's a nearby thread that's addressing this same question, in which
> I make the case (again) that the right thing for postgres_fdw constraints
> is that they're just assumed true. So I'm not sure why this conversation
> is proposing to implement a lot of mechanism to do something different
> from that.

/me scratches head.

Because the other guy named Tom Lane took the opposite position on the
second message on this thread, dated 11/14/13?

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

(2014/01/22 4:09), Robert Haas wrote:
> On Mon, Jan 20, 2014 at 9:44 PM, Shigeru Hanada
> <shigeru(dot)hanada(at)gmail(dot)com> wrote:
>> Thanks for the comments.
>>
>> 2014/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>> In addition, an idea which I can't throw away is to assume that all
>>>> constraints defined on foreign tables as ASSERTIVE. Foreign tables
>>>> potentially have dangers to have "wrong" data by updating source data
>>>> not through foreign tables. This is not specific to an FDW, so IMO
>>>> constraints defined on foreign tables are basically ASSERTIVE. Of
>>>> course PG can try to maintain data correct, but always somebody might
>>>> break it.
>>>> qu
>>>>
>>> Does it make sense to apply "assertive" CHECK constraint on the qual
>>> of ForeignScan to filter out tuples with violated values at the local
>>> side, as if row-level security feature doing.
>>> It enables to handle a situation that planner expects only "clean"
>>> tuples are returned but FDW driver is unavailable to anomalies.
>>>
>>> Probably, this additional check can be turned on/off on the fly,
>>> if FDW driver has a way to inform the core system its capability,
>>> like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip
>>> local checks.
>>
>> Hmm, IIUC you mean that local users can't (or don't need to) know that
>> data which violates the local constraints exist on remote side.
>> Applying constraints to the data which is modified through FDW would
>> be necessary as well. In that design, FDW is a bidirectional filter
>> which provides these features:
>>
>> 1) Don't push wrong data into remote data source, by applying local
>> constraints to the result of the modifying query executed on local PG.
>> This is not perfect filter, because remote constraints don't mapped
>> automatically or perfectly (imagine constraints which is available on
>> remote but is not supported in PG).
>> 2) Don't retrieve wrong data from remote to local PG, by applying
>> local constraints
>>
>> I have a concern about consistency. It has not been supported, but
>> let's think of Aggregate push-down invoked by a query below.
>>
>> SELECT count(*) FROM remote_table;
>>
>> If this query was fully pushed down, the result is the # of records
>> exist on remote side, but the result would be # of valid records when
>> we don't push down the aggregate. This would confuse users.
>>
>>>> Besides CHECK constraints, currently NOT NULL constraints are
>>>> virtually ASSERTIVE (not enforcing). Should it also be noted
>>>> explicitly?
>>>>
>>> Backward compatibility….
>>
>> Yep, backward compatibility (especially visible ones to users) should
>> be minimal, ideally zero.
>>
>>> NOT NULL [ASSERTIVE] might be an option.
>>
>> Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow
>> ingASSERTIVE for only foreign tables? It makes sense, though we need
>> consider exclusiveness . But It needs to default to ASSERTIVE on
>> foreign tables, and NOT ASSERTIVE (means "forced") on others. Isn't
>> is too complicated?
>>
>> CREATE FOREIGN TABLE foo (
>> id int NOT NULL ASSERTIVE CHECK (id > 1) ASSERTIVE,
>> …
>> CONSTRAINT chk_foo_name_upper CHECK (upper(name) = name) ASSERTIVE
>> ) SERVER server;
>>
>> BTW, I noticed that this is like push-down-able expressions in
>> JOIN/WHERE. We need to check a CHECK constraint defined on a foreign
>> tables contains only expressions which have same semantics as remote
>> side (in practice, built-in and immutable)?
>
> I don't think that that ASSERTIVE is going to fly, because "assertive"
> means (sayeth the Google) "having or showing a confident and forceful
> personality", which is not what we mean here. It's tempting to do
> something like try to replace the keyword "check" with "assume" or
> "assert" or (stretching) "assertion", but that would require whichever
> one we picked to be a fully-reserved keyword, which I can't think is
> going to get much support here, for entirely understandable reasons.
> So I think we should look for another option.
>
> Currently, constraints can be marked NO INHERIT (though this seems to
> have not been fully documented, as the ALTER TABLE page doesn't
> mention it anywhere) or NOT VALID, so I'm thinking maybe we should go
> with something along those lines. Some ideas:
>
> - NO CHECK. The idea of writing CHECK (id > 1) NO CHECK is pretty
> hilarious, though.
> - NO VALIDATE. But then people need to understand that NOT VALID
> means "we didn't validate it yet" while "no validate" means "we don't
> ever intend to validate it", which could be confusing.
> - NO ENFORCE. Requires a new (probably unreserved) keyword.
> - NOT VALIDATED or NOT CHECKED. Same problems as NO CHECK and NO
> VALIDATE, respectively, plus now we have to create a new keyword.
>
> Another idea is to apply an extensible-options syntax to constraints,
> like we do for EXPLAIN, VACUUM, etc. Like maybe:
>
> CHECK (id > 1) OPTIONS (enforced false, valid true)
>
> Yet another idea is to consider validity a three-state property:
> either the constraint is valid (because we have checked it and are
> enforcing it), or it is not valid (because we are enforcing it but
> have not checked the pre-existing data), or it is assumed true
> (because we are not checking or enforcing it but are believing it
> anyway). So then we could have a syntax like this:
>
> CHECK (id > 1) VALIDATE { ON | OFF | ASSERTION }
>
> Other ideas?
>
> One thing that's bugging me a bit about this whole line of attack is
> that, in the first instance, the whole goal here is to support
> inheritance hierarchies that mix ordinary tables with foreign tables.
> If you have a table with children some of which are inherited and
> others of which are not inherited, you're very likely going to want
> your constraints enforced for real on the children that are tables and
> assumed true on the children that are foreign tables, and none of what
> we're talking about here gets us to that, because we normally want the
> constraints to be identical throughout the inheritance hierarchy.
> Maybe there's some way around that, but I'm back to wondering if it
> wouldn't be better to simply silently force any constraints on a
> foreign-table into assertion mode. That could be done without any new
> syntax at all, and frankly I think it's what people are going to want
> more often than not.

I'd like to vote for the idea of silently forcing any constraints on a
foreign-table into assertion mode. No new syntax and better documentation.

Thanks,

Best regards,
Etsuro Fujita

Hi Hanada-san,

While still reviwing this patch, I feel this patch has given enough
consideration to interactions with other commands, but found the
following incorrect? behabior:

postgres=# CREATE TABLE product (id INTEGER, description TEXT);
CREATE TABLE
postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product) SERVER fs
OPTIONS (filename '/home/foo/product1.csv', format 'csv');
CREATE FOREIGN TABLE
postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
EXTERNAL;
ERROR: "product1" is not a table or materialized view

ISTN the ALTER TABLE simple recursion mechanism (ie ATSimpleRecursion())
should be modified for the ALTER COLUMN SET STORAGE case.

I just wanted to quickly tell you this for you to take time to consider.

Thanks,

Best regards,
Etsuro Fujita

2014-01-27 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> While still reviwing this patch, I feel this patch has given enough
> consideration to interactions with other commands, but found the following
> incorrect? behabior:
>
> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
> CREATE TABLE
> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product) SERVER fs
> OPTIONS (filename '/home/foo/product1.csv', format 'csv');
> CREATE FOREIGN TABLE
> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
> EXTERNAL;
> ERROR: "product1" is not a table or materialized view
>
> ISTN the ALTER TABLE simple recursion mechanism (ie ATSimpleRecursion())
> should be modified for the ALTER COLUMN SET STORAGE case.
>
> I just wanted to quickly tell you this for you to take time to consider.

Thanks for the review. It must be an oversight, so I'll fix it up soon.

--
Shigeru HANADA

On Mon, Jan 27, 2014 at 05:06:19PM +0900, Etsuro Fujita wrote:
> Hi Hanada-san,
>
> While still reviwing this patch, I feel this patch has given enough
> consideration to interactions with other commands, but found the
> following incorrect? behabior:
>
> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
> CREATE TABLE
> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product)
> SERVER fs OPTIONS (filename '/home/foo/product1.csv', format 'csv');
> CREATE FOREIGN TABLE
> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
> EXTERNAL;
> ERROR: "product1" is not a table or materialized view
>
> ISTN the ALTER TABLE simple recursion mechanism (ie
> ATSimpleRecursion()) should be modified for the ALTER COLUMN SET
> STORAGE case.

This points to a larger discussion about what precisely foreign tables
can and cannot inherit from local ones. I don't think that a generic
solution will be satisfactory, as the PostgreSQL FDW could, at least
in principle, support many more than the CSV FDW, as shown above.

In my estimation, the outcome of discussion above is not a blocker for
this 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

Sent from my iPad

> On 27-Jan-2014, at 21:03, David Fetter <david(at)fetter(dot)org> wrote:
>
>> On Mon, Jan 27, 2014 at 05:06:19PM +0900, Etsuro Fujita wrote:
>> Hi Hanada-san,
>>
>> While still reviwing this patch, I feel this patch has given enough
>> consideration to interactions with other commands, but found the
>> following incorrect? behabior:
>>
>> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
>> CREATE TABLE
>> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product)
>> SERVER fs OPTIONS (filename '/home/foo/product1.csv', format 'csv');
>> CREATE FOREIGN TABLE
>> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
>> EXTERNAL;
>> ERROR: "product1" is not a table or materialized view
>>
>> ISTN the ALTER TABLE simple recursion mechanism (ie
>> ATSimpleRecursion()) should be modified for the ALTER COLUMN SET
>> STORAGE case.
>
> This points to a larger discussion about what precisely foreign tables
> can and cannot inherit from local ones. I don't think that a generic
> solution will be satisfactory, as the PostgreSQL FDW could, at least
> in principle, support many more than the CSV FDW, as shown above.
>
> In my estimation, the outcome of discussion above is not a blocker for
> this

I wonder what shall be the cases when foreign table is on a server which does not support *all* SQL features.

Does a FDW need to have the possible inherit options mentioned in its documentation for this patch?

Regards,

Atri

(2014/01/28 0:55), Atri Sharma wrote:
>> On 27-Jan-2014, at 21:03, David Fetter <david(at)fetter(dot)org> wrote:
>>> On Mon, Jan 27, 2014 at 05:06:19PM +0900, Etsuro Fujita wrote:
>>> Hi Hanada-san,
>>>
>>> While still reviwing this patch, I feel this patch has given enough
>>> consideration to interactions with other commands, but found the
>>> following incorrect? behabior:
>>>
>>> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
>>> CREATE TABLE
>>> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product)
>>> SERVER fs OPTIONS (filename '/home/foo/product1.csv', format 'csv');
>>> CREATE FOREIGN TABLE
>>> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
>>> EXTERNAL;
>>> ERROR: "product1" is not a table or materialized view
>>>
>>> ISTN the ALTER TABLE simple recursion mechanism (ie
>>> ATSimpleRecursion()) should be modified for the ALTER COLUMN SET
>>> STORAGE case.
>>
>> This points to a larger discussion about what precisely foreign tables
>> can and cannot inherit from local ones. I don't think that a generic
>> solution will be satisfactory, as the PostgreSQL FDW could, at least
>> in principle, support many more than the CSV FDW, as shown above.
>>
>> In my estimation, the outcome of discussion above is not a blocker for
>> this

I just thought that among the structures that local tables can alter,
the ones that foreign tables also can by ALTER FOREIGN TABLE are
inherited, and the others are not inherited. So for the case as shown
above, I thought that we silently ignore executing the ALTER COLUMN SET
STORAGE command for the foreign table. I wonder that would be the first
step.

> I wonder what shall be the cases when foreign table is on a server which does not support *all* SQL features.
>
> Does a FDW need to have the possible inherit options mentioned in its documentation for this patch?

The answer is no, in my understanding. The altering operation simply
declares some chages for foreign tables in the inheritance tree and does
nothing to the underlying storages.

Thanks,

Best regards,
Etsuro Fujita

> > I wonder what shall be the cases when foreign table is on a server which
> does not support *all* SQL features.
> >
> > Does a FDW need to have the possible inherit options mentioned in its
> documentation for this patch?
>
> The answer is no, in my understanding. The altering operation simply
> declares some chages for foreign tables in the inheritance tree and does
> nothing to the underlying storages.
>
It seems to me Atri mention about the idea to enforce constraint when
partitioned foreign table was referenced...

BTW, isn't it feasible to consign FDW drivers its decision?
If a foreign table has a check constraint (X BETWEEN 101 AND 200),
postgres_fdw will be capable to run this check on the remote server,
however, file_fdw will not be capable. It is usual job of them when
qualifiers are attached on Path node.
Probably, things to do is simple. If the backend appends the configured
check constraint as if a user-given qualifier, FDW driver will handle it
appropriately.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Etsuro Fujita
> Sent: Tuesday, January 28, 2014 1:08 PM
> To: Atri Sharma; David Fetter
> Cc: pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] inherit support for foreign tables
>
> (2014/01/28 0:55), Atri Sharma wrote:
> >> On 27-Jan-2014, at 21:03, David Fetter <david(at)fetter(dot)org> wrote:
> >>> On Mon, Jan 27, 2014 at 05:06:19PM +0900, Etsuro Fujita wrote:
> >>> Hi Hanada-san,
> >>>
> >>> While still reviwing this patch, I feel this patch has given enough
> >>> consideration to interactions with other commands, but found the
> >>> following incorrect? behabior:
> >>>
> >>> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
> >>> CREATE TABLE postgres=# CREATE FOREIGN TABLE product1 () INHERITS
> >>> (product) SERVER fs OPTIONS (filename '/home/foo/product1.csv',
> >>> format 'csv'); CREATE FOREIGN TABLE postgres=# ALTER TABLE product
> >>> ALTER COLUMN description SET STORAGE EXTERNAL;
> >>> ERROR: "product1" is not a table or materialized view
> >>>
> >>> ISTN the ALTER TABLE simple recursion mechanism (ie
> >>> ATSimpleRecursion()) should be modified for the ALTER COLUMN SET
> >>> STORAGE case.
> >>
> >> This points to a larger discussion about what precisely foreign
> >> tables can and cannot inherit from local ones. I don't think that a
> >> generic solution will be satisfactory, as the PostgreSQL FDW could,
> >> at least in principle, support many more than the CSV FDW, as shown above.
> >>
> >> In my estimation, the outcome of discussion above is not a blocker
> >> for this
>
> I just thought that among the structures that local tables can alter, the
> ones that foreign tables also can by ALTER FOREIGN TABLE are inherited,
> and the others are not inherited. So for the case as shown above, I thought
> that we silently ignore executing the ALTER COLUMN SET STORAGE command for
> the foreign table. I wonder that would be the first step.
>
> > I wonder what shall be the cases when foreign table is on a server which
> does not support *all* SQL features.
> >
> > Does a FDW need to have the possible inherit options mentioned in its
> documentation for this patch?
>
> The answer is no, in my understanding. The altering operation simply
> declares some chages for foreign tables in the inheritance tree and does
> nothing to the underlying storages.
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org) To make
> changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

(2014/01/27 21:52), Shigeru Hanada wrote:
> 2014-01-27 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> While still reviwing this patch, I feel this patch has given enough
>> consideration to interactions with other commands, but found the following
>> incorrect? behabior:
>>
>> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
>> CREATE TABLE
>> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product) SERVER fs
>> OPTIONS (filename '/home/foo/product1.csv', format 'csv');
>> CREATE FOREIGN TABLE
>> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
>> EXTERNAL;
>> ERROR: "product1" is not a table or materialized view
>>
>> ISTN the ALTER TABLE simple recursion mechanism (ie ATSimpleRecursion())
>> should be modified for the ALTER COLUMN SET STORAGE case.
>>
>> I just wanted to quickly tell you this for you to take time to consider.
>
> Thanks for the review. It must be an oversight, so I'll fix it up soon.
>

It seems little bit complex than I expected. Currently foreign tables
deny ALTER TABLE SET STORAGE with message like below, because foreign
tables don't have storage in the meaning of PG heap tables.

ERROR: "pgbench1_accounts_c1" is not a table or materialized view

At the moment we don't use attstorage for foreign tables, so allowing
SET STORAGE against foreign tables never introduce visible change
except \d+ output of foreign tables. But IMO such operation should
not allowed because users would be confused. So I changed
ATExecSetStorage() to skip on foreign tables. This allows us to emit
ALTER TABLE SET STORAGE against ordinary tables in upper level of
inheritance tree, but it have effect on only ordinary tables in the
tree.

This also allows direct ALTER FOREIGN TABLE SET STORAGE against
foreign table but the command is silently ignored. SET STORAGE
support for foreign tables is not documented because it may confuse
users.

With attached patch, SET STORAGE against wrong relations produces
message like this. Is it confusing to mention foreign table here?

ERROR: "pgbench1_accounts_pkey" is not a table, materialized view, or
foreign table

One other idea is to support SET STORAGE against foreign tables though
it has no effect. This makes all tables and foreign tables to have
same storage option in same column. It might be more understandable
behavior for users.

Thoughts?

FYI, here are lists of ALTER TABLE features which is allowed/denied
for foreign tables.

Allowed
- ADD|DROP COLUMN
- SET DATA TYPE
- SET|DROP NOT NULL
- SET STATISTICS
- SET (attribute_option = value)
- RESET (atrribute_option)
- SET|DROP DEFAULT
- ADD table_constraint
- ALTER CONSTRAINT
- DROP CONSTRAINT
- [NO] INHERIT parent_table
- OWNER
- RENAME
- SET SCHEMA
- SET STORAGE
- moved to here by attached patch

Denied
- ADD table_constraint_using_index
- VALIDATE CONSTRAINT
- DISABLE|ENABLE TRIGGER
- DISABLE|ENABLE RULE
- CLUSTER ON
- SET WITHOUT CLUSTER
- SET WITH|WITHOUT OIDS
- SET (storage_parameter = value)
- RESET (storage_parameter)
- OF type
- NOT OF
- SET TABLESPACE
- REPLICA IDENTITY

--
Shigeru HANADA

--
Shigeru HANADA

Sent from my iPad

On 28-Jan-2014, at 10:04, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

>>> I wonder what shall be the cases when foreign table is on a server which
>> does not support *all* SQL features.
>>>
>>> Does a FDW need to have the possible inherit options mentioned in its
>> documentation for this patch?
>>
>> The answer is no, in my understanding. The altering operation simply
>> declares some chages for foreign tables in the inheritance tree and does
>> nothing to the underlying storages.
> It seems to me Atri mention about the idea to enforce constraint when
> partitioned foreign table was referenced...
>
> BTW, isn't it feasible to consign FDW drivers its decision?
> If a foreign table has a check constraint (X BETWEEN 101 AND 200),
> postgres_fdw will be capable to run this check on the remote server,
> however, file_fdw will not be capable. It is usual job of them when
> qualifiers are attached on Path node.
> Probably, things to do is simple. If the backend appends the configured
> check constraint as if a user-given qualifier, FDW driver will handle

Hi,

I think that pushing it to FDW driver is the best approach. The FDW driver knows whether or not the foreign server supports the said feature,hence,the user should be abstracted from that.

I agree that the foreign constraint can be added as a user defined qualifier and dealt with accordingly.

Regards,

Atri

Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
> At the moment we don't use attstorage for foreign tables, so allowing
> SET STORAGE against foreign tables never introduce visible change
> except \d+ output of foreign tables. But IMO such operation should
> not allowed because users would be confused. So I changed
> ATExecSetStorage() to skip on foreign tables.

I think this is totally misguided. Who's to say that some weird FDW
might not pay attention to attstorage? I could imagine a file-based
FDW using that to decide whether to compress columns, for instance.
Admittedly, the chances of that aren't large, but it's pretty hard
to argue that going out of our way to prevent it is a useful activity.

regards, tom lane

On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> writes:
>> At the moment we don't use attstorage for foreign tables, so allowing
>> SET STORAGE against foreign tables never introduce visible change
>> except \d+ output of foreign tables. But IMO such operation should
>> not allowed because users would be confused. So I changed
>> ATExecSetStorage() to skip on foreign tables.
>
> I think this is totally misguided. Who's to say that some weird FDW
> might not pay attention to attstorage? I could imagine a file-based
> FDW using that to decide whether to compress columns, for instance.
> Admittedly, the chances of that aren't large, but it's pretty hard
> to argue that going out of our way to prevent it is a useful activity.

I think that's a pretty tenuous position. There are already
FDW-specific options sufficient to let a particular FDW store whatever
kinds of options it likes; letting the user set options that were only
ever intended to be applied to tables just because we can seems sort
of dubious. I'm tempted by the idea of continuing to disallow SET
STORAGE on an unvarnished foreign table, but allowing it on an
inheritance hierarchy that contains at least one real table, with the
semantics that we quietly ignore the foreign tables and apply the
operation to the plain tables.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think this is totally misguided. Who's to say that some weird FDW
>> might not pay attention to attstorage? I could imagine a file-based
>> FDW using that to decide whether to compress columns, for instance.
>> Admittedly, the chances of that aren't large, but it's pretty hard
>> to argue that going out of our way to prevent it is a useful activity.

> I think that's a pretty tenuous position. There are already
> FDW-specific options sufficient to let a particular FDW store whatever
> kinds of options it likes; letting the user set options that were only
> ever intended to be applied to tables just because we can seems sort
> of dubious. I'm tempted by the idea of continuing to disallow SET
> STORAGE on an unvarnished foreign table, but allowing it on an
> inheritance hierarchy that contains at least one real table, with the
> semantics that we quietly ignore the foreign tables and apply the
> operation to the plain tables.

[ shrug... ] By far the easiest implementation of that is just to apply
the catalog change to all of them. According to your assumptions, it'll
be a no-op on the foreign tables anyway.

regards, tom lane

On Thu, Jan 30, 2014 at 5:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I think this is totally misguided. Who's to say that some weird FDW
>>> might not pay attention to attstorage? I could imagine a file-based
>>> FDW using that to decide whether to compress columns, for instance.
>>> Admittedly, the chances of that aren't large, but it's pretty hard
>>> to argue that going out of our way to prevent it is a useful activity.
>
>> I think that's a pretty tenuous position. There are already
>> FDW-specific options sufficient to let a particular FDW store whatever
>> kinds of options it likes; letting the user set options that were only
>> ever intended to be applied to tables just because we can seems sort
>> of dubious. I'm tempted by the idea of continuing to disallow SET
>> STORAGE on an unvarnished foreign table, but allowing it on an
>> inheritance hierarchy that contains at least one real table, with the
>> semantics that we quietly ignore the foreign tables and apply the
>> operation to the plain tables.
>
> [ shrug... ] By far the easiest implementation of that is just to apply
> the catalog change to all of them. According to your assumptions, it'll
> be a no-op on the foreign tables anyway.

Well, there's some point to that, too, I suppose. What do others think?

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Jan 30, 2014 at 5:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> I think this is totally misguided. Who's to say that some weird FDW
> >>> might not pay attention to attstorage? I could imagine a file-based
> >>> FDW using that to decide whether to compress columns, for instance.
> >>> Admittedly, the chances of that aren't large, but it's pretty hard
> >>> to argue that going out of our way to prevent it is a useful activity.
> >
> >> I think that's a pretty tenuous position. There are already
> >> FDW-specific options sufficient to let a particular FDW store whatever
> >> kinds of options it likes; letting the user set options that were only
> >> ever intended to be applied to tables just because we can seems sort
> >> of dubious. I'm tempted by the idea of continuing to disallow SET
> >> STORAGE on an unvarnished foreign table, but allowing it on an
> >> inheritance hierarchy that contains at least one real table, with the
> >> semantics that we quietly ignore the foreign tables and apply the
> >> operation to the plain tables.
> >
> > [ shrug... ] By far the easiest implementation of that is just to apply
> > the catalog change to all of them. According to your assumptions, it'll
> > be a no-op on the foreign tables anyway.
>
> Well, there's some point to that, too, I suppose. What do others think?

I agree that using the FDW-specific options is the right approach and
disallowing those to be set on foreign tables makes sense. I don't
particularly like the idea of applying changes during inheiritance
which we wouldn't allow the user to do directly. While it might be a
no-op and no FDW might use it, it'd still be confusing.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I agree that using the FDW-specific options is the right approach and
> disallowing those to be set on foreign tables makes sense. I don't
> particularly like the idea of applying changes during inheiritance
> which we wouldn't allow the user to do directly. While it might be a
> no-op and no FDW might use it, it'd still be confusing.

If we don't allow it directly, why not? Seems rather arbitrary.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I agree that using the FDW-specific options is the right approach and
> > disallowing those to be set on foreign tables makes sense. I don't
> > particularly like the idea of applying changes during inheiritance
> > which we wouldn't allow the user to do directly. While it might be a
> > no-op and no FDW might use it, it'd still be confusing.
>
> If we don't allow it directly, why not? Seems rather arbitrary.

I'm apparently missing something here. From my perspective, they're
either allowed or they're not and if they aren't allowed then they
shouldn't be allowed to happen. I'd view this like a CHECK constraint-
if it's a foreign table then it can't have a value for SET STORAGE. I
don't see why it would be 'ok' to allow it to be set if we're setting it
as part of inheiritance but otherwise not allow it to be set.

Thanks,

Stephen

(2014/01/31 9:56), Robert Haas wrote:
> On Thu, Jan 30, 2014 at 5:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Thu, Jan 30, 2014 at 11:04 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> I think this is totally misguided. Who's to say that some weird FDW
>>>> might not pay attention to attstorage? I could imagine a file-based
>>>> FDW using that to decide whether to compress columns, for instance.
>>>> Admittedly, the chances of that aren't large, but it's pretty hard
>>>> to argue that going out of our way to prevent it is a useful activity.
>>
>>> I think that's a pretty tenuous position. There are already
>>> FDW-specific options sufficient to let a particular FDW store whatever
>>> kinds of options it likes; letting the user set options that were only
>>> ever intended to be applied to tables just because we can seems sort
>>> of dubious. I'm tempted by the idea of continuing to disallow SET
>>> STORAGE on an unvarnished foreign table, but allowing it on an
>>> inheritance hierarchy that contains at least one real table, with the
>>> semantics that we quietly ignore the foreign tables and apply the
>>> operation to the plain tables.
>>
>> [ shrug... ] By far the easiest implementation of that is just to apply
>> the catalog change to all of them. According to your assumptions, it'll
>> be a no-op on the foreign tables anyway.
>
> Well, there's some point to that, too, I suppose. What do others think?

Allowing ALTER COLUMN SET STORAGE on foreign tables would make sense if
for example, "SELECT * INTO local_table FROM foreign_table" did create a
new local table of columns having the storage types associated with
those of a foreign table?

Thanks,

Best regards,
Etsuro Fujita

On Sun, Feb 2, 2014 at 10:15 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Allowing ALTER COLUMN SET STORAGE on foreign tables would make sense if for
> example, "SELECT * INTO local_table FROM foreign_table" did create a new
> local table of columns having the storage types associated with those of a
> foreign table?

Seems like a pretty weak argument. It's not that we can't find
strange corner cases where applying SET STORAGE to a foreign table
doesn't do something; it's that they *are* strange corner cases. The
options as we normally don't understand them just aren't sensible in
this context, and a good deal of work has been put into an alternative
options framework, which is what authors of FDWs ought to be using.

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

(2014/02/04 20:56), Robert Haas wrote:
> On Sun, Feb 2, 2014 at 10:15 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Allowing ALTER COLUMN SET STORAGE on foreign tables would make sense if for
>> example, "SELECT * INTO local_table FROM foreign_table" did create a new
>> local table of columns having the storage types associated with those of a
>> foreign table?
>
> Seems like a pretty weak argument. It's not that we can't find
> strange corner cases where applying SET STORAGE to a foreign table
> doesn't do something; it's that they *are* strange corner cases. The
> options as we normally don't understand them just aren't sensible in
> this context, and a good deal of work has been put into an alternative
> options framework, which is what authors of FDWs ought to be using.

I just wanted to discuss the possiblity of allowing SET STORAGE on a
foreign table, but I've got the point. I'll resume the patch review.

Thanks,

Best regards,
Etsuro Fujita

Hi Hanada-san,

Sorry for the delay.

(2014/01/30 14:01), Shigeru Hanada wrote:
>> 2014-01-27 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>>> While still reviwing this patch, I feel this patch has given enough
>>> consideration to interactions with other commands, but found the following
>>> incorrect? behabior:
>>>
>>> postgres=# CREATE TABLE product (id INTEGER, description TEXT);
>>> CREATE TABLE
>>> postgres=# CREATE FOREIGN TABLE product1 () INHERITS (product) SERVER fs
>>> OPTIONS (filename '/home/foo/product1.csv', format 'csv');
>>> CREATE FOREIGN TABLE
>>> postgres=# ALTER TABLE product ALTER COLUMN description SET STORAGE
>>> EXTERNAL;
>>> ERROR: "product1" is not a table or materialized view
>>>
>>> ISTN the ALTER TABLE simple recursion mechanism (ie ATSimpleRecursion())
>>> should be modified for the ALTER COLUMN SET STORAGE case.

> It seems little bit complex than I expected. Currently foreign tables
> deny ALTER TABLE SET STORAGE with message like below, because foreign
> tables don't have storage in the meaning of PG heap tables.
>
> ERROR: "pgbench1_accounts_c1" is not a table or materialized view
>
> At the moment we don't use attstorage for foreign tables, so allowing
> SET STORAGE against foreign tables never introduce visible change
> except \d+ output of foreign tables. But IMO such operation should
> not allowed because users would be confused. So I changed
> ATExecSetStorage() to skip on foreign tables. This allows us to emit
> ALTER TABLE SET STORAGE against ordinary tables in upper level of
> inheritance tree, but it have effect on only ordinary tables in the
> tree.
>
> This also allows direct ALTER FOREIGN TABLE SET STORAGE against
> foreign table but the command is silently ignored. SET STORAGE
> support for foreign tables is not documented because it may confuse
> users.
>
> With attached patch, SET STORAGE against wrong relations produces
> message like this. Is it confusing to mention foreign table here?
>
> ERROR: "pgbench1_accounts_pkey" is not a table, materialized view, or
> foreign table

ITSM that would be confusing to users. So, I've modified the patch so
that we continue to disallow SET STORAGE on a foreign table *in the same
manner as before*, but, as your patch does, allow it on an inheritance
hierarchy that contains foreign tables, with the semantics that we
quietly ignore the foreign tables and apply the operation to the plain
tables, by modifying the ALTER TABLE simple recursion mechanism.
Attached is the updated version of the patch.

I'll continue the patch review a bit more, though the patch looks good
generally to me except for the abobe issue and the way that the ANALYZE
command works. For the latter, if there are no objections, I'll merge
the ANALYZE patch [1] with your patch.

Thanks,

[1] http://www.postgresql.org/message-id/52EB10AC.4070307@lab.ntt.co.jp

Best regards,
Etsuro Fujita

(2014/02/07 21:31), Etsuro Fujita wrote:
> So, I've modified the patch so
> that we continue to disallow SET STORAGE on a foreign table *in the same
> manner as before*, but, as your patch does, allow it on an inheritance
> hierarchy that contains foreign tables, with the semantics that we
> quietly ignore the foreign tables and apply the operation to the plain
> tables, by modifying the ALTER TABLE simple recursion mechanism.
> Attached is the updated version of the patch.
>
> I'll continue the patch review a bit more, though the patch looks good
> generally to me except for the abobe issue and the way that the ANALYZE
> command works.

While reviwing the patch, I've found some issues on interactions with
other commands, other than the SET STORAGE command.

* ADD table_constraint NOT VALID: the patch allows ADD table_constraint
*NOT VALID* to be set on a foreign table as well as an inheritance
hierarchy that contains foreign tables. But I think it would be better
to disallow ADD table_constraint *NOT VALID* on a foreign table, but
allow it on an inheritance hierarchy that contains foreign tables, with
the semantics that we apply the operation to the plain tables and apply
the transformed operation *ADD table_constraint* to the foreign tables.

* VALIDATE CONSTRAINT constraint_name: though the patch disallow the
direct operation on foreign tables, it allows the operation on an
inheritance hierarchy that contains foreign tables, and the operation
fails if there is at least one foreign table that has at least one NOT
VALID constraint. I think it would be better to modify the patch so
that we allow the operation on an inheritance hierarchy that contains
foreign tables, with the semantics that we quietly ignore the foreign
tables and apply the operation on the plain tables just like the
semantics of SET STORAGE. (Note that the foreign tables wouldn't have
NOT VALID constraints by diallowing ADD table_constraint *NOT VALID* on
foreign tables as mentioned above.)

* SET WITH OIDS: though the patch disallow the direct operation on
foreign tables, it allows the operation on an inheritance hierarchy that
contains foreign tables, and that operation is successfully done on
foreign tables. I think it would be better to modify the patch so that
we allow it on an inheritance hierarchy that contains foreign tables,
with the semantics that we quietly ignore the foreign tables and apply
the operation to the plain tables just like the semantics of SET STORAGE.

Comments are wellcome!

Thanks,

Best regards,
Etsuro Fujita

(2014/02/10 21:00), Etsuro Fujita wrote:
> (2014/02/07 21:31), Etsuro Fujita wrote:
>> So, I've modified the patch so
>> that we continue to disallow SET STORAGE on a foreign table *in the same
>> manner as before*, but, as your patch does, allow it on an inheritance
>> hierarchy that contains foreign tables, with the semantics that we
>> quietly ignore the foreign tables and apply the operation to the plain
>> tables, by modifying the ALTER TABLE simple recursion mechanism.

> While reviwing the patch, I've found some issues on interactions with
> other commands, other than the SET STORAGE command.

> * ADD table_constraint NOT VALID: the patch allows ADD table_constraint
> *NOT VALID* to be set on a foreign table as well as an inheritance
> hierarchy that contains foreign tables. But I think it would be better
> to disallow ADD table_constraint *NOT VALID* on a foreign table, but
> allow it on an inheritance hierarchy that contains foreign tables, with
> the semantics that we apply the operation to the plain tables and apply
> the transformed operation *ADD table_constraint* to the foreign tables.

Done.

> * VALIDATE CONSTRAINT constraint_name:

I've modified the patch so that though we continue to disallow the
operation on a foreign table, we allow it on an inheritance hierarchy
that contains foreign tables. In that case, the to-be-validated
constraints on the plain tables are validated by the operation, as
before, and the constraints on the foreign tables are ignored. Note
that the constraints on the foreign tables are not NOT VALID due to the
spec mentioned above.

> * SET WITH OIDS: though the patch disallow the direct operation on
> foreign tables, it allows the operation on an inheritance hierarchy that
> contains foreign tables, and that operation is successfully done on
> foreign tables. I think it would be better to modify the patch so that
> we allow it on an inheritance hierarchy that contains foreign tables,
> with the semantics that we quietly ignore the foreign tables and apply
> the operation to the plain tables just like the semantics of SET STORAGE.

I noticed this breaks inheritance hierarchy. To avoid this, I've
modified the patch to disallow SET WITH OIDS on an inheritance hierarchy
that contains at least one foreign table.

The other changes:

- if (stmt->constraints != NIL && relkind == RELKIND_FOREIGN_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("constraints are not supported
on foreign tables")));
+/*
+ * Shouldn't this have been checked in parser?
+ */
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ {
+ ListCell *lc;
+ foreach(lc, stmt->constraints)
+ {
+ NewConstraint *nc = lfirst(lc);
+
+ if (nc->contype != CONSTR_CHECK &&
+ nc->contype != CONSTR_DEFAULT &&
+ nc->contype != CONSTR_NULL &&
+ nc->contype != CONSTR_NOTNULL)
+ ereport(ERROR,
+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("only check
constraints are supported on foreign tables")));
+ }
+ }

ISTM we wouldn't need the above check in DefineRelation(), so I've
removed the check from the patch. And I've modified the vague error
messages in parse_utilcmd.c.

There seems to be no objections, I've merged the ANALYZE patch [1] with
your patch. I noticed that the patch in [1] isn't efficient. (To
ANALYZE one inheritance tree that contains foreign tables, the patch in
[1] calls the AnalyzeForeignTable() routine two times for each such
foreign table.) So, the updated version has been merged that calls the
routine only once for each such foreign table.

Todo:

ISTM the documentation would need to be updated further.

Thanks,

[1] http://www.postgresql.org/message-id/52EB10AC.4070307@lab.ntt.co.jp

Best regards,
Etsuro Fujita

Hi Fujita-san,

Thanks for the reviewing.

2014-02-10 21:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/02/07 21:31), Etsuro Fujita wrote:
>> So, I've modified the patch so
>> that we continue to disallow SET STORAGE on a foreign table *in the same
>> manner as before*, but, as your patch does, allow it on an inheritance
>> hierarchy that contains foreign tables, with the semantics that we
>> quietly ignore the foreign tables and apply the operation to the plain
>> tables, by modifying the ALTER TABLE simple recursion mechanism.
>> Attached is the updated version of the patch.

I'm not sure that allowing ALTER TABLE against parent table affects
descendants even some of them are foreign table. I think the rule
should be simple enough to understand for users, of course it should
be also consistent and have backward compatibility.

If foreign table can be modified through inheritance tree, this kind
of change can be done.

1) create foreign table as a child of a ordinary table
2) run ALTER TABLE parent, the foreign table is also changed
3) remove foreign table from the inheritance tree by ALTER TABLE child
NO INHERIT parent
4) here we can't do same thing as 2), because it is not a child anymore.

So IMO we should determine which ALTER TABLE features are allowed to
foreign tables, and allow them regardless of the recursivity.

Comments?
--
Shigeru HANADA

> From: Shigeru Hanada [mailto:shigeru(dot)hanada(at)gmail(dot)com]

> 2014-02-10 21:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> > (2014/02/07 21:31), Etsuro Fujita wrote:
> >> So, I've modified the patch so
> >> that we continue to disallow SET STORAGE on a foreign table *in the
> >> same manner as before*, but, as your patch does, allow it on an
> >> inheritance hierarchy that contains foreign tables, with the
> >> semantics that we quietly ignore the foreign tables and apply the
> >> operation to the plain tables, by modifying the ALTER TABLE simple
> recursion mechanism.
> >> Attached is the updated version of the patch.
>
> I'm not sure that allowing ALTER TABLE against parent table affects
> descendants even some of them are foreign table. I think the rule should
> be simple enough to understand for users, of course it should be also
> consistent and have backward compatibility.

Yeah, the understandability is important. But I think the flexibility is also important. In other words, I think it is a bit too inflexible that we disallow e.g., SET STORAGE to be set on an inheritance tree that contains foreign table(s) because we disallow SET STORAGE to be set on foreign tables directly.

> If foreign table can be modified through inheritance tree, this kind of
> change can be done.
>
> 1) create foreign table as a child of a ordinary table
> 2) run ALTER TABLE parent, the foreign table is also changed
> 3) remove foreign table from the inheritance tree by ALTER TABLE child NO
> INHERIT parent
> 4) here we can't do same thing as 2), because it is not a child anymore.
>
> So IMO we should determine which ALTER TABLE features are allowed to foreign
> tables, and allow them regardless of the recursivity.

What I think should be newly allowed to be set on foreign tables is

* ADD table_constraint
* DROP CONSTRAINT
* [NO] INHERIT parent_table

Thanks,

Best regards,
Etsuro Fujita

Hello,

> 2014-02-10 21:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> > (2014/02/07 21:31), Etsuro Fujita wrote:
> >> So, I've modified the patch so
> >> that we continue to disallow SET STORAGE on a foreign table *in the same
> >> manner as before*, but, as your patch does, allow it on an inheritance
> >> hierarchy that contains foreign tables, with the semantics that we
> >> quietly ignore the foreign tables and apply the operation to the plain
> >> tables, by modifying the ALTER TABLE simple recursion mechanism.
> >> Attached is the updated version of the patch.
>
> I'm not sure that allowing ALTER TABLE against parent table affects
> descendants even some of them are foreign table. I think the rule
> should be simple enough to understand for users, of course it should
> be also consistent and have backward compatibility.

Could you guess any use cases in which we are happy with ALTER
TABLE's inheritance tree walking? IMHO, ALTER FOREIGN TABLE
always comes with some changes of the data source so implicitly
invoking of such commands should be defaultly turned off. If the
ALTER'ing the whole familiy is deadfully useful for certain
cases, it might be explicitly turned on by some syntax added to
CREATE FOREIGN TABLE or ALTER TABLE.

It would looks like following,

CREATE FOREIGN TABLE ft1 () INHERITS (pt1 ALLOW_INHERITED_ALTER, pt2);

ALTER TABLE INCLUDE FOREIGN CHILDREN parent ADD COLUMN add1 integer;

These looks quite bad :-( but also seem quite better than
accidentially ALTER'ing foreign children.

If foreign tables were allowed to ALTER'ed with 'ALTER TABLE',
some reconstruction between 'ALTER TABLE' and 'ALTER FOREIGN
TABLE' would be needed.

> If foreign table can be modified through inheritance tree, this kind
> of change can be done.
>
> 1) create foreign table as a child of a ordinary table
> 2) run ALTER TABLE parent, the foreign table is also changed
> 3) remove foreign table from the inheritance tree by ALTER TABLE child
> NO INHERIT parent
> 4) here we can't do same thing as 2), because it is not a child anymore.
>
> So IMO we should determine which ALTER TABLE features are allowed to
> foreign tables, and allow them regardless of the recursivity.
>
> Comments?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> Could you guess any use cases in which we are happy with ALTER
> TABLE's inheritance tree walking? IMHO, ALTER FOREIGN TABLE
> always comes with some changes of the data source so implicitly
> invoking of such commands should be defaultly turned off. If the
> ALTER'ing the whole familiy is deadfully useful for certain
> cases, it might be explicitly turned on by some syntax added to
> CREATE FOREIGN TABLE or ALTER TABLE.

> It would looks like following,

> CREATE FOREIGN TABLE ft1 () INHERITS (pt1 ALLOW_INHERITED_ALTER, pt2);

> ALTER TABLE INCLUDE FOREIGN CHILDREN parent ADD COLUMN add1 integer;

Ugh. This adds a lot of logical complication without much real use,
IMO. The problems that have been discussed in this thread are that
certain types of ALTER are sensible to apply to foreign tables and
others are not --- so how does a one-size-fits-all switch help that?

Also, there are some types of ALTER for which recursion to children
*must* occur or things become inconsistent --- ADD COLUMN itself is
an example, and ADD CONSTRAINT is another. It would not be acceptable
for an ALLOW_INHERITED_ALTER switch to suppress that, but if the
switch is leaky then it's even less consistent and harder to explain.

regards, tom lane

Hello,

At Tue, 18 Feb 2014 09:49:23 -0500, Tom Lane wrote
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > Could you guess any use cases in which we are happy with ALTER
> > TABLE's inheritance tree walking? IMHO, ALTER FOREIGN TABLE
> > always comes with some changes of the data source so implicitly
> > invoking of such commands should be defaultly turned off. If the
> > ALTER'ing the whole familiy is deadfully useful for certain
> > cases, it might be explicitly turned on by some syntax added to
> > CREATE FOREIGN TABLE or ALTER TABLE.
>
> > It would looks like following,
>
> > CREATE FOREIGN TABLE ft1 () INHERITS (pt1 ALLOW_INHERITED_ALTER, pt2);
>
> > ALTER TABLE INCLUDE FOREIGN CHILDREN parent ADD COLUMN add1 integer;
>
> Ugh. This adds a lot of logical complication without much real use,
> IMO.

Yeah! That's exactry the words I wanted for the expamples. Sorry
for bothering you. I also don't see any use case driving us to
overcome the extra complexity.

> The problems that have been discussed in this thread are that
> certain types of ALTER are sensible to apply to foreign tables and
> others are not --- so how does a one-size-fits-all switch help that?

None, I think. I intended only to display what this ALTER's
inheritance walkthrough brings in.

> Also, there are some types of ALTER for which recursion to children
> *must* occur or things become inconsistent --- ADD COLUMN itself is
> an example, and ADD CONSTRAINT is another. It would not be acceptable
> for an ALLOW_INHERITED_ALTER switch to suppress that, but if the
> switch is leaky then it's even less consistent and harder to explain.

Exactly. It is the problem came with allowing to implicit ALTER
on foreign children, Shigeru's last message seems to referred to.

At Tue, 18 Feb 2014 14:47:51 +0900, Shigeru Hanada wrote
> I'm not sure that allowing ALTER TABLE against parent table affects
> descendants even some of them are foreign table.

Avoiding misunderstandings, my opinion on whether ALTER may
applie to foreign chidlren is 'no'.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Tue, 18 Feb 2014 19:24:50 +0900, "Etsuro Fujita" wrote
> > From: Shigeru Hanada [mailto:shigeru(dot)hanada(at)gmail(dot)com]
> > 2014-02-10 21:00 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> > > (2014/02/07 21:31), Etsuro Fujita wrote:
> > >> So, I've modified the patch so
> > >> that we continue to disallow SET STORAGE on a foreign table *in the
> > >> same manner as before*, but, as your patch does, allow it on an
> > >> inheritance hierarchy that contains foreign tables, with the
> > >> semantics that we quietly ignore the foreign tables and apply the
> > >> operation to the plain tables, by modifying the ALTER TABLE simple
> > recursion mechanism.
> > >> Attached is the updated version of the patch.
> >
> > I'm not sure that allowing ALTER TABLE against parent table affects
> > descendants even some of them are foreign table. I think the rule should
> > be simple enough to understand for users, of course it should be also
> > consistent and have backward compatibility.
>
> Yeah, the understandability is important. But I think the
> flexibility is also important. In other words, I think it is a
> bit too inflexible that we disallow e.g., SET STORAGE to be set
> on an inheritance tree that contains foreign table(s) because
> we disallow SET STORAGE to be set on foreign tables directly.

What use case needs such a flexibility precedes the lost behavior
predictivity of ALTER TABLE and/or code "maintenancibility"(more
ordinally words must be...) ? I don't agree with the idea that
ALTER TABLE implicitly affects foreign children for the reason in
the upthread. Also turning on/off feature implemented as special
syntax seems little hope.

If you still want that, I suppose ALTER FOREIGN TABLE is usable
chainging so as to walk through the inheritance tree and affects
only foreign tables along the way. It can naturally affects
foregin tables with no unanticipated behavor.

Thoughts?

> > If foreign table can be modified through inheritance tree, this kind of
> > change can be done.
> >
> > 1) create foreign table as a child of a ordinary table
> > 2) run ALTER TABLE parent, the foreign table is also changed
> > 3) remove foreign table from the inheritance tree by ALTER TABLE child NO
> > INHERIT parent
> > 4) here we can't do same thing as 2), because it is not a child anymore.
> >
> > So IMO we should determine which ALTER TABLE features are allowed to foreign
> > tables, and allow them regardless of the recursivity.
>
> What I think should be newly allowed to be set on foreign tables is
>
> * ADD table_constraint
> * DROP CONSTRAINT

As of 9.3

http://www.postgresql.org/docs/9.3/static/sql-alterforeigntable.html

> Consistency with the foreign server is not checked when a
> column is added or removed with ADD COLUMN or DROP COLUMN, a
> NOT NULL constraint is added, or a column type is changed with
> SET DATA TYPE. It is the user's responsibility to ensure that
> the table definition matches the remote side.

So I belive implicit and automatic application of any constraint
on foreign childs are considerably danger.

> * [NO] INHERIT parent_table

Is this usable for inheritance foreign children? NO INHERIT
removes all foreign children but INHERIT is nop.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Horiguchi-san,

2014-02-18 19:29 GMT+09:00 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
> Could you guess any use cases in which we are happy with ALTER
> TABLE's inheritance tree walking? IMHO, ALTER FOREIGN TABLE
> always comes with some changes of the data source so implicitly
> invoking of such commands should be defaultly turned off.

Imagine a case that foreign data source have attributes (A, B, C, D)
but foreign tables and their parent ware defined as (A, B, C). If
user wants to use D as well, ALTER TABLE parent ADD COLUMN D type
would be useful (rather necessary?) to keep consistency.

Changing data type from compatible one (i.e., int to numeric,
varchar(n) to text), adding CHECK/NOT NULL constraint would be also
possible.

--
Shigeru HANADA

(2014/02/19 12:12), Kyotaro HORIGUCHI wrote:
> At Tue, 18 Feb 2014 19:24:50 +0900, "Etsuro Fujita" wrote
>>> From: Shigeru Hanada [mailto:shigeru(dot)hanada(at)gmail(dot)com]

>>> I'm not sure that allowing ALTER TABLE against parent table affects
>>> descendants even some of them are foreign table. I think the rule should
>>> be simple enough to understand for users, of course it should be also
>>> consistent and have backward compatibility.
>>
>> Yeah, the understandability is important. But I think the
>> flexibility is also important. In other words, I think it is a
>> bit too inflexible that we disallow e.g., SET STORAGE to be set
>> on an inheritance tree that contains foreign table(s) because
>> we disallow SET STORAGE to be set on foreign tables directly.
>
> What use case needs such a flexibility precedes the lost behavior
> predictivity of ALTER TABLE and/or code "maintenancibility"(more
> ordinally words must be...) ? I don't agree with the idea that
> ALTER TABLE implicitly affects foreign children for the reason in
> the upthread. Also turning on/off feature implemented as special
> syntax seems little hope.

It is just my personal opinion, but I think it would be convenient for
users to alter inheritance trees that contain foreign tables the same
way as inheritance trees that don't contain any foreign tables, without
making user conscious of the inheritance trees contains foreign tables
or not. Imagine we have an inheritance tree that contains only plain
tables and then add a foreign table as a child of the inheritance tree.
Without the flexiblity, we would need to change the way of altering
the structure of the inheritance tree (e.g., ADD CONSTRAINT) to a
totally different one, immediately when adding the foreign table. I
don't think that would be easy to use.

>> What I think should be newly allowed to be set on foreign tables is
>>
>> * ADD table_constraint
>> * DROP CONSTRAINT
>
> As of 9.3
>
> http://www.postgresql.org/docs/9.3/static/sql-alterforeigntable.html
>
>> Consistency with the foreign server is not checked when a
>> column is added or removed with ADD COLUMN or DROP COLUMN, a
>> NOT NULL constraint is added, or a column type is changed with
>> SET DATA TYPE. It is the user's responsibility to ensure that
>> the table definition matches the remote side.
>
> So I belive implicit and automatic application of any constraint
> on foreign childs are considerably danger.

We spent a lot of time discussing this issue, and the consensus is that
it's users' fault if there are some tuples on the remote side violating
a given constraint, as mentioned in the documentation.

>> * [NO] INHERIT parent_table
>
> Is this usable for inheritance foreign children? NO INHERIT
> removes all foreign children but INHERIT is nop.

I didn't express clearly. Sorry for that. Let me explain about it.

* ALTER FOREIGN TABLE target_table *INHERIT* parent_table: Add the
target table as a new child of the parent table.
* ALTER FOREIGN TABLE target_table *NO INHERIT* parent_table: Remove the
target table from the list of children of the parent table.

Thanks,

Best regards,
Etsuro Fujita

Hello,

> It is just my personal opinion, but I think it would be convenient for
> users to alter inheritance trees that contain foreign tables the same
> way as inheritance trees that don't contain any foreign tables,
> without making user conscious of the inheritance trees contains
> foreign tables or not. Imagine we have an inheritance tree that
> contains only plain tables and then add a foreign table as a child of
> the inheritance tree. Without the flexiblity, we would need to change
> the way of altering the structure of the inheritance tree (e.g., ADD
> CONSTRAINT) to a totally different one, immediately when adding the
> foreign table. I don't think that would be easy to use.

I personally don't see significant advantages such a
flexibility. Although my concerns here are only two points,
unanticipated application and "maintenancibility". I gave a
consideration on these issues again.

Then, I think it could be enough by giving feedback to operators
for the first issue.

=# ALTER TABLE parent ADD CHECK (tempmin < tempmax),
ALTER tempmin SET NOT NULL,
ALTER tempmin SET DEFAULT 0;
NOTICE: Child foregn table child01 is affected.
NOTICE: Child foregn table child02 is affected
NOTICE: Child foregn table child03 rejected 'alter tempmin set default'

What do you think about this? It looks a bit too loud for me
though...

Then the second issue, however I don't have enough idea of how
ALTER TABLE works, the complexity would be reduced if acceptance
chek for alter "action"s would done on foreign server or data
wrapper side, not on the core of ALTER TABLE. It would also be a
help to output error messages like above. However, (NO)INHERIT
looks a little different..

> > http://www.postgresql.org/docs/9.3/static/sql-alterforeigntable.html
> >
> >> Consistency with the foreign server is not checked when a
> >> column is added or removed with ADD COLUMN or DROP COLUMN, a
> >> NOT NULL constraint is added, or a column type is changed with
> >> SET DATA TYPE. It is the user's responsibility to ensure that
> >> the table definition matches the remote side.
> >
> > So I belive implicit and automatic application of any constraint
> > on foreign childs are considerably danger.
>
> We spent a lot of time discussing this issue, and the consensus is
> that it's users' fault if there are some tuples on the remote side
> violating a given constraint, as mentioned in the documentation.

I'm worried about not that but the changes and possible
inconsistency would take place behind operators' backs. And this
looks to cause such inconsistencies for me.

> >> * [NO] INHERIT parent_table
> >
> > Is this usable for inheritance foreign children? NO INHERIT
> > removes all foreign children but INHERIT is nop.
>
> I didn't express clearly. Sorry for that. Let me explain about it.
>
> * ALTER FOREIGN TABLE target_table *INHERIT* parent_table: Add the
> * target table as a new child of the parent table.
> * ALTER FOREIGN TABLE target_table *NO INHERIT* parent_table: Remove the
> * target table from the list of children of the parent table.

I got it, thank you. It alone seems no probmen but also doesn't
seem to be a matter of 'ALTER TABLE'. Could you tell me how this
is related to 'ALTER TABLE'?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

At Wed, 19 Feb 2014 16:17:05 +0900, Shigeru Hanada wrote
> 2014-02-18 19:29 GMT+09:00 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
> > Could you guess any use cases in which we are happy with ALTER
> > TABLE's inheritance tree walking? IMHO, ALTER FOREIGN TABLE
> > always comes with some changes of the data source so implicitly
> > invoking of such commands should be defaultly turned off.
>
> Imagine a case that foreign data source have attributes (A, B, C, D)
> but foreign tables and their parent ware defined as (A, B, C). If
> user wants to use D as well, ALTER TABLE parent ADD COLUMN D type
> would be useful (rather necessary?) to keep consistency.

Hmm. I seems to me an issue of mis-configuration at first
step. However, my anxiety is - as in my message just before -
ALTER'ing foreign table definitions without any notice to
operatos and irregular or random logic on check applicability(?)
of ALTER actions.

> Changing data type from compatible one (i.e., int to numeric,
> varchar(n) to text), adding CHECK/NOT NULL constraint would be also
> possible.

I see, thank you. Changing data types are surely valuable but
also seems difficult to check validity:(

Anyway, I gave a second thought on this issue. Please have a look
on that.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/02/20 15:47), Kyotaro HORIGUCHI wrote:
> Although my concerns here are only two points,
> unanticipated application and "maintenancibility". I gave a
> consideration on these issues again.

Sorry, I misunderstood what you mean by "unanticipated application".

> Then, I think it could be enough by giving feedback to operators
> for the first issue.
>
> =# ALTER TABLE parent ADD CHECK (tempmin < tempmax),
> ALTER tempmin SET NOT NULL,
> ALTER tempmin SET DEFAULT 0;
> NOTICE: Child foregn table child01 is affected.
> NOTICE: Child foregn table child02 is affected
> NOTICE: Child foregn table child03 rejected 'alter tempmin set default'
>
> What do you think about this? It looks a bit too loud for me
> though...

I think that's a good idea. What do others think?

> Then the second issue, however I don't have enough idea of how
> ALTER TABLE works, the complexity would be reduced if acceptance
> chek for alter "action"s would done on foreign server or data
> wrapper side, not on the core of ALTER TABLE. It would also be a
> help to output error messages like above.

I'm not sure it's worth having such an mechanism inside/outside the PG
core. I might misunderstand your concern here, but is it the risk of
constraint violation? If so, I'd like to vote for an idea of avoiding
that violation by making the FDW in itself perform ExecQual() for each
tuple retrived from the remote server at the query time.

>> We spent a lot of time discussing this issue, and the consensus is
>> that it's users' fault if there are some tuples on the remote side
>> violating a given constraint, as mentioned in the documentation.
>
> I'm worried about not that but the changes and possible
> inconsistency would take place behind operators' backs. And this
> looks to cause such inconsistencies for me.

That is what you mean by "unanticipated application", right?

>>>> * [NO] INHERIT parent_table
>>>
>>> Is this usable for inheritance foreign children? NO INHERIT
>>> removes all foreign children but INHERIT is nop.
>>
>> I didn't express clearly. Sorry for that. Let me explain about it.
>>
>> * ALTER FOREIGN TABLE target_table *INHERIT* parent_table: Add the
>> * target table as a new child of the parent table.
>> * ALTER FOREIGN TABLE target_table *NO INHERIT* parent_table: Remove the
>> * target table from the list of children of the parent table.
>
> I got it, thank you. It alone seems no probmen but also doesn't
> seem to be a matter of 'ALTER TABLE'. Could you tell me how this
> is related to 'ALTER TABLE'?

These are not related to ALTER TABLE. [NO] INHERIT parent_table (and
ADD/DROP CONSTRAINT) are what I think should be newly allowed to apply
to foreign tables directly.

Thanks,

Best regards,
Etsuro Fujita

(2014/02/20 19:55), Etsuro Fujita wrote:
> (2014/02/20 15:47), Kyotaro HORIGUCHI wrote:
>> Although my concerns here are only two points,
>> unanticipated application and "maintenancibility". I gave a
>> consideration on these issues again.
>
> Sorry, I misunderstood what you mean by "unanticipated application".
>
>> Then, I think it could be enough by giving feedback to operators
>> for the first issue.
>>
>> =# ALTER TABLE parent ADD CHECK (tempmin < tempmax),
>> ALTER tempmin SET NOT NULL,
>> ALTER tempmin SET DEFAULT 0;
>> NOTICE: Child foregn table child01 is affected.
>> NOTICE: Child foregn table child02 is affected
>> NOTICE: Child foregn table child03 rejected 'alter tempmin set default'
>>
>> What do you think about this? It looks a bit too loud for me
>> though...
>
> I think that's a good idea.

I just thought those messages would be shown for the user to readily
notice the changes of the structures of child tables that are foreign,
done by the recursive altering operation. But I overlooked the third line:

NOTICE: Child foregn table child03 rejected 'alter tempmin set default'

What does "rejected" in this message mean?

Thanks,

Best regards,
Etsuro Fujita

Hi,

> >> NOTICE: Child foregn table child01 is affected.
> >> NOTICE: Child foregn table child02 is affected
> >> NOTICE: Child foregn table child03 rejected 'alter tempmin set
> >> default'
> >>
> >> What do you think about this? It looks a bit too loud for me
> >> though...
> >
> > I think that's a good idea.
>
> I just thought those messages would be shown for the user to readily
> notice the changes of the structures of child tables that are foreign,
> done by the recursive altering operation. But I overlooked the third
> line:
>
> NOTICE: Child foregn table child03 rejected 'alter tempmin set
> default'
>
> What does "rejected" in this message mean?

It says that child03 had no ability to perform the requested
action, in this case setting a default value. It might be better
to reject ALTER on the parent as a whole when any children
doesn't accept any action.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

> > Then the second issue, however I don't have enough idea of how
> > ALTER TABLE works, the complexity would be reduced if acceptance
> > chek for alter "action"s would done on foreign server or data
> > wrapper side, not on the core of ALTER TABLE. It would also be a
> > help to output error messages like above.
>
> I'm not sure it's worth having such an mechanism inside/outside the PG
> core. I might misunderstand your concern here, but is it the risk of
> constraint violation?

A bit different:) It's the problem of how and who decides whether
each ALTER action given can be performed on each child. Set of
available actions vary according to the nature of each foreign
table/server/driver and also according to their functional
evolutions made in future, largely independent of the core,
maybe. The core oughtn't to.. couldn't maintain such a function
judging availability of every possible combination of action and
foreign table.

> If so, I'd like to vote for an idea of avoiding
> that violation by making the FDW in itself perform ExecQual() for each
> tuple retrived from the remote server at the query time.

In my humble opition, it is not so serious problem that
functionally acceptable actions can cause any kind of
inconsistency by the nature of fdw so far. It is well documented
and operators should be aware of such inconsistencies after being
informed of what they did, by the NOTICE messages. I think.

> >> * ALTER FOREIGN TABLE target_table *INHERIT* parent_table: Add the
> >> * target table as a new child of the parent table.
> >> * ALTER FOREIGN TABLE target_table *NO INHERIT* parent_table: Remove the
> >> * target table from the list of children of the parent table.
> >
> > I got it, thank you. It alone seems no probmen but also doesn't
> > seem to be a matter of 'ALTER TABLE'. Could you tell me how this
> > is related to 'ALTER TABLE'?
>
> These are not related to ALTER TABLE. [NO] INHERIT parent_table (and
> ADD/DROP CONSTRAINT) are what I think should be newly allowed to apply
> to foreign tables directly.

Thank you, I understood that. I thought it already existed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/02/21 15:23), Kyotaro HORIGUCHI wrote:
>>>> NOTICE: Child foregn table child01 is affected.
>>>> NOTICE: Child foregn table child02 is affected
>>>> NOTICE: Child foregn table child03 rejected 'alter tempmin set
>>>> default'
>>>>
>>>> What do you think about this? It looks a bit too loud for me
>>>> though...
>>>
>>> I think that's a good idea.
>>
>> I just thought those messages would be shown for the user to readily
>> notice the changes of the structures of child tables that are foreign,
>> done by the recursive altering operation. But I overlooked the third
>> line:
>>
>> NOTICE: Child foregn table child03 rejected 'alter tempmin set
>> default'
>>
>> What does "rejected" in this message mean?
>
> It says that child03 had no ability to perform the requested
> action, in this case setting a default value. It might be better
> to reject ALTER on the parent as a whole when any children
> doesn't accept any action.

Now understood, thougn I'm not sure it's worth implementing such a
checking mechanism in the recursive altering operation...

Thanks,

Best regards,
Etsuro Fujita

Hello,

At Fri, 21 Feb 2014 16:33:32 +0900, Etsuro Fujita wrote
> (2014/02/21 15:23), Kyotaro HORIGUCHI wrote:
> >>>> NOTICE: Child foregn table child01 is affected.
> >>>> NOTICE: Child foregn table child02 is affected
> >>>> NOTICE: Child foregn table child03 rejected 'alter tempmin set
> >>>> default'
> > It says that child03 had no ability to perform the requested
> > action, in this case setting a default value. It might be better
> > to reject ALTER on the parent as a whole when any children
> > doesn't accept any action.
>
> Now understood, thougn I'm not sure it's worth implementing such a
> checking mechanism in the recursive altering operation...

Did you mean foreign tables can sometimes silently ignore ALTER
actions which it can't perform? It will cause inconsistency which
operators didn't anticipate. This example uses "add column" for
perspicuitly but all types of action could result like this.

==============
=# ALTER TABLE parent ADD COLUMN x integer;
ALTER TABLE
=# \d parent
Table "public.parent"
Column | Type | Modifiers
--------+---------+--------------------
a | integer |
b | integer |
x | integer |
Number of child tables: 2 (Use \d+ to list them.)
=# \d child1
Foreign table "public.child1"
Column | Type | Modifiers | FDW Options
----------+---------+-----------+-------------
a | integer |
b | integer |

=# (Op: Ouch!)
==============

I think this should result as,

==============
=# ALTER TABLE parent ADD COLUMN x integer;
ERROR: Foreign child table child1 could not perform some of the actions.
=#
==============

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

In addition to an issue pointed out recently by Horiguchi-san, I've
found there is another issue we have to discuss. That is, we can't
build any parameterized Append paths for an inheritance tree that
contains at least one foreign table, in set_append_rel_pathlist(). This
is because the patch doesn't take into consideration
"reparameterization" for paths for a foreign table, which attempts to
modify these paths to have greater parameterization so that each child
path in the inheritance tree can have the exact same parameterization.
To do so, I think we would need to add code for the foreign-table case
to reparameterize_path(). And I think we should introduce a new FDW
routine, say ReparameterizeForeignPath() because the processing would be
performed best by the FDW itself.

Comments are welcome!

Thanks,

Best regards,
Etsuro Fujita

Hello, I tried minimal implementation to do that.

At Tue, 25 Feb 2014 19:45:56 +0900, Etsuro Fujita wrote
> In addition to an issue pointed out recently by Horiguchi-san, I've
> found there is another issue we have to discuss. That is, we can't
> build any parameterized Append paths for an inheritance tree that
> contains at least one foreign table, in set_append_rel_pathlist(). This
> is because the patch doesn't take into consideration
> "reparameterization" for paths for a foreign table, which attempts to
> modify these paths to have greater parameterization so that each child
> path in the inheritance tree can have the exact same parameterization.
> To do so, I think we would need to add code for the foreign-table case
> to reparameterize_path(). And I think we should introduce a new FDW
> routine, say ReparameterizeForeignPath() because the processing would be
> performed best by the FDW itself.
>
> Comments are welcome!

I think the problem is foreign childs in inheritance tables
prevents all menber in the inheritance relation from using
parameterized paths, correct?

|=# explain select * from p join (select uname from c1 limit 1) s on s.uname = p.uname;
| QUERY PLAN
|-------------------------------------------------------------------------------
| Hash Join (cost=0.04..244.10 rows=50 width=58)
| Hash Cond: (p.uname = c1_1.uname)
| -> Append (cost=0.00..206.01 rows=10012 width=50)
| -> Seq Scan on p (cost=0.00..0.00 rows=1 width=168)
| -> Seq Scan on c1 (cost=0.00..204.01 rows=10001 width=50)
| -> Foreign Scan on c2 (cost=0.00..2.00 rows=10 width=168)
| Foreign File: /etc/passwd
| Foreign File Size: 1851
| -> Hash (cost=0.03..0.03 rows=1 width=8)
| -> Limit (cost=0.00..0.02 rows=1 width=8)
| -> Seq Scan on c1 c1_1 (cost=0.00..204.01 rows=10001 width=8)
| Planning time: 1.095 ms

Hmm. I tried minimal implementation to do that. This omits cost
recalculation but seems to work as expected. This seems enough if
cost recalc is trivial here.

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index b79af7a..18ced04 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2062,6 +2062,16 @@ reparameterize_path(PlannerInfo *root, Path *path,
case T_SubqueryScan:
return create_subqueryscan_path(root, rel, path->pathkeys,
required_outer);
+ case T_ForeignScan:
+ {
+ ForeignPath *fpath = (ForeignPath*) path;
+ ForeignPath *newpath = makeNode(ForeignPath);
+ memcpy(newpath, fpath, sizeof(ForeignPath));
+ newpath->path.param_info =
+ get_baserel_parampathinfo(root, rel, required_outer);
+ /* cost recalc omitted */
+ return (Path *)newpath;
+ }
default:
break;
}
....

|=# explain select * from p join (select uname from c1 limit 1) s on s.uname = p.uname;
| QUERY PLAN
|----------------------------------------------------------------------------
| Nested Loop (cost=0.00..10.46 rows=50 width=58)
| -> Limit (cost=0.00..0.02 rows=1 width=8)
| -> Seq Scan on c1 c1_1 (cost=0.00..204.01 rows=10001 width=8)
| -> Append (cost=0.00..10.30 rows=12 width=158)
| -> Seq Scan on p (cost=0.00..0.00 rows=1 width=168)
| Filter: (c1_1.uname = uname)
| -> Index Scan using i_c1 on c1 (cost=0.29..8.30 rows=1 width=50)
| Index Cond: (uname = c1_1.uname)
| -> Foreign Scan on c2 (cost=0.00..2.00 rows=10 width=168)
| Filter: (c1_1.uname = uname)
| Foreign File: /etc/passwd
| Foreign File Size: 1851
| Planning time: 2.044 ms

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. As a minimal implementation, I made an attempt that emit
NOTICE message when alter table affects foreign tables. It looks
like following,

| =# alter table passwd add column added int, add column added2 int;
| NOTICE: This command affects foreign relation "cf1"
| NOTICE: This command affects foreign relation "cf1"
| ALTER TABLE
| =# select * from passwd;
| ERROR: missing data for column "added"
| CONTEXT: COPY cf1, line 1: "root:x:0:0:root:/root:/bin/bash"
| =#

This seems far better than silently performing the command,
except for the duplicated message:( New bitmap might required to
avoid the duplication..

I made the changes above and below as an attempt in the attached
patch foreign_inherit-v4.patch

> I think the problem is foreign childs in inheritance tables
> prevents all menber in the inheritance relation from using
> parameterized paths, correct?
...
> Hmm. I tried minimal implementation to do that. This omits cost
> recalculation but seems to work as expected. This seems enough if
> cost recalc is trivial here.

Any comments?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

> This seems far better than silently performing the command,
> except for the duplicated message:( New bitmap might required to
> avoid the duplication..

I rewrote it in more tidy way. ATController collects all affected
tables on ATRewriteCatalogs as first stage, then emit NOTICE
message according to the affected relations list. The message
looks like,

| =# alter table passwd alter column uname set default 'hoge';
| NOTICE: This command affects 2 foreign tables: cf1, cf2
| ALTER TABLE

Do you feel this too large or complicated? I think so a bit..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/03/11 14:07), Kyotaro HORIGUCHI wrote:
>> This seems far better than silently performing the command,
>> except for the duplicated message:( New bitmap might required to
>> avoid the duplication..
>
> I rewrote it in more tidy way. ATController collects all affected
> tables on ATRewriteCatalogs as first stage, then emit NOTICE
> message according to the affected relations list. The message
> looks like,
>
> | =# alter table passwd alter column uname set default 'hoge';
> | NOTICE: This command affects 2 foreign tables: cf1, cf2
> | ALTER TABLE
>
> Do you feel this too large or complicated? I think so a bit..

No. I think that would be a useful message for the user.

My feeling is it would be better to show this kind of messages for all
the affected tables whether or not the affected ones are foreign. How
about introducing a VERBOSE option for ALTER TABLE? Though, I think
that should be implemented as a separate patch.

Thanks,

Best regards,
Etsuro Fujita

Hi Horiguchi-san,

Thank you for working this patch!

(2014/03/10 17:29), Kyotaro HORIGUCHI wrote:
> Hello. As a minimal implementation, I made an attempt that emit
> NOTICE message when alter table affects foreign tables. It looks
> like following,
>
> | =# alter table passwd add column added int, add column added2 int;
> | NOTICE: This command affects foreign relation "cf1"
> | NOTICE: This command affects foreign relation "cf1"
> | ALTER TABLE
> | =# select * from passwd;
> | ERROR: missing data for column "added"
> | CONTEXT: COPY cf1, line 1: "root:x:0:0:root:/root:/bin/bash"
> | =#
>
> This seems far better than silently performing the command,
> except for the duplicated message:( New bitmap might required to
> avoid the duplication..

As I said before, I think it would be better to show this kind of
information on each of the affected tables whether or not the affected
one is foreign. I also think it would be better to show it only when
the user has specified an option to do so, similar to a VERBOSE option
of other commands. ISTM this should be implemented as a separate patch.

> I made the changes above and below as an attempt in the attached
> patch foreign_inherit-v4.patch
>
>> I think the problem is foreign childs in inheritance tables
>> prevents all menber in the inheritance relation from using
>> parameterized paths, correct?

Yes, I think so, too.

>> Hmm. I tried minimal implementation to do that. This omits cost
>> recalculation but seems to work as expected. This seems enough if
>> cost recalc is trivial here.

I think we should redo the cost/size estimate, because for example,
greater parameterization leads to a smaller rowcount estimate, if I
understand correctly. In addition, I think this reparameterization
should be done by the FDW itself, becasuse the FDW has more knowledge
about it than the PG core. So, I think we should introduce a new FDW
routine for that, say ReparameterizeForeignPath(), as proposed in [1].
Attached is an updated version of the patch. Due to the above reason, I
removed from the patch the message displaying function you added.

Sorry for the delay.

[1] http://www.postgresql.org/message-id/530C7464.6020006@lab.ntt.co.jp

Best regards,
Etsuro Fujita

Hi Fujita-san,

> Thank you for working this patch!

No problem, but my point seems always out of the main target a bit:(

> > | =# alter table passwd add column added int, add column added2 int;
> > | NOTICE: This command affects foreign relation "cf1"
> > | NOTICE: This command affects foreign relation "cf1"
> > | ALTER TABLE
> > | =# select * from passwd;
> > | ERROR: missing data for column "added"
> > | CONTEXT: COPY cf1, line 1: "root:x:0:0:root:/root:/bin/bash"
> > | =#
> >
> > This seems far better than silently performing the command,
> > except for the duplicated message:( New bitmap might required to
> > avoid the duplication..
>
> As I said before, I think it would be better to show this kind of
> information on each of the affected tables whether or not the affected
> one is foreign. I also think it would be better to show it only when
> the user has specified an option to do so, similar to a VERBOSE option
> of other commands. ISTM this should be implemented as a separate
> patch.

Hmm. I *wish* this kind of indication to be with this patch even
only for foreign tables which would have inconsistency
potentially. Expanding to other objects and/or knobs are no
problem to be added later.

> >> Hmm. I tried minimal implementation to do that. This omits cost
> >> recalculation but seems to work as expected. This seems enough if
> >> cost recalc is trivial here.
>
> I think we should redo the cost/size estimate, because for example,
> greater parameterization leads to a smaller rowcount estimate, if I
> understand correctly. In addition, I think this reparameterization
> should be done by the FDW itself, becasuse the FDW has more knowledge
> about it than the PG core. So, I think we should introduce a new FDW
> routine for that, say ReparameterizeForeignPath(), as proposed in
> [1]. Attached is an updated version of the patch. Due to the above
> reason, I removed from the patch the message displaying function you
> added.
>
> Sorry for the delay.
>
> [1]
> http://www.postgresql.org/message-id/530C7464.6020006@lab.ntt.co.jp

I had a rough look on foreign reparameterize stuff. It seems ok
generally.

By the way, Can I have a simple script to build an environment to
run this on?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

> By the way, Can I have a simple script to build an environment to
> run this on?

I built test environment and ran the simple test using
postgres_fdw and got parameterized path from v3 patch on the
following operation as shown there, and v6 also gives one, but I
haven't seen the reparameterization of v6 patch work.

# How could I think to have got it work before?

Do you have any idea to make postgreReparameterizeForeignPath on
foreign (child) tables works effectively?

regards,

====
### on pg1/pg2:
create table pu1 (a int not null, b int not null, c int, d text);
create unique index i_pu1_ab on pu1 (a, b);
create unique index i_pu1_c on pu1 (c);
create table cu11 (like pu1 including all) inherits (pu1);
create table cu12 (like pu1 including all) inherits (pu1);
insert into cu11 (select a / 5, 4 - (a % 5), a, 'cu11' from generate_series(000000, 099999) a);
insert into cu12 (select a / 5, 4 - (a % 5), a, 'cu12' from generate_series(100000, 199999) a);

### on pg1:
create extension postgres_fdw;
create server pg2 foreign data wrapper postgres_fdw options (host '/tmp', port '5433', dbname 'postgres');
create user mapping for current_user server pg2 options (user 'horiguti');
create foreign table _cu11 (a int, b int, c int, d text) server pg2 options (table_name 'cu11', use_remote_estimate 'true');
create foreign table _cu12 (a int, b int, c int, d text) server pg2 options (table_name 'cu12', use_remote_estimate 'true');
create table rpu1 (a int, b int, c int, d text);
alter foreign table _cu11 inherit rpu1;
alter foreign table _cu12 inherit rpu1;
analyze rpu1;

=# explain analyze select pu1.*
from pu1 join rpu1 on (pu1.c = rpu1.c) where pu1.a = 3;
QUERY PLAN
-------------------------------------------------------------------------------
Nested Loop (cost=0.00..2414.57 rows=11 width=19)
(actual time=0.710..7.167 rows=5 loops=1)
-> Append (cost=0.00..30.76 rows=11 width=19)
<local side.. ommitted>
-> Append (cost=0.00..216.68 rows=3 width=4)
(actual time=0.726..1.419 rows=1 loops=5)
-> Seq Scan on rpu1 (cost=0.00..0.00 rows=1 width=4)
(actual time=0.000..0.000 rows=0 loops=5)
Filter: (pu1.c = c)
-> Foreign Scan on _cu11 (cost=100.30..108.34 rows=1 width=4)
(actual time=0.602..0.603 rows=1 loops=5)
-> Foreign Scan on _cu12 (cost=100.30..108.34 rows=1 width=4)
(actual time=0.566..0.566 rows=0 loops=5)
Planning time: 4.452 ms
Total runtime: 7.663 ms
(15 rows)

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/03/18 18:38), Kyotaro HORIGUCHI wrote:
>> By the way, Can I have a simple script to build an environment to
>> run this on?
>
> I built test environment and ran the simple test using
> postgres_fdw and got parameterized path from v3 patch on the
> following operation as shown there, and v6 also gives one, but I
> haven't seen the reparameterization of v6 patch work.
>
> # How could I think to have got it work before?
>
> Do you have any idea to make postgreReparameterizeForeignPath on
> foreign (child) tables works effectively?

> =# explain analyze select pu1.*
> from pu1 join rpu1 on (pu1.c = rpu1.c) where pu1.a = 3;

ISTM postgresReparameterizeForeignPath() cannot be called in this query
in principle. Here is a simple example for the case where the
use_remote_estimate option is true:

# On mydatabase

mydatabase=# CREATE TABLE mytable (id INTEGER, x INTEGER);
CREATE TABLE
mydatabase=# INSERT INTO mytable SELECT x, x FROM generate_series(0,
9999) x;
INSERT 0 10000

# On postgres

postgres=# CREATE TABLE inttable (id INTEGER);
CREATE TABLE
postgres=# INSERT INTO inttable SELECT x FROM generate_series(0, 9999) x;
INSERT 0 10000
postgres=# ANALYZE inttable;
ANALYZE

postgres=# CREATE TABLE patest0 (id INTEGER, x INTEGER);
CREATE TABLE
postgres=# CREATE TABLE patest1 () INHERITS (patest0);
CREATE TABLE
postgres=# INSERT INTO patest1 SELECT x, x FROM generate_series(0, 9999) x;
INSERT 0 10000
postgres=# CREATE INDEX patest1_id_idx ON patest1(id);
CREATE INDEX
postgres=# CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (host 'localhost', dbname 'mydatabase');
CREATE SERVER
postgres=# CREATE USER MAPPING FOR PUBLIC SERVER myserver OPTIONS (user
'pgsql');
CREATE USER MAPPING
postgres=# CREATE FOREIGN TABLE patest2 () INHERITS (patest0) SERVER
myserver OPTIONS (table_name 'mytable');
CREATE FOREIGN TABLE
postgres=# ANALYZE patest0;
ANALYZE
postgres=# ANALYZE patest1;
ANALYZE
postgres=# ANALYZE patest2;
ANALYZE
postgres=# EXPLAIN VERBOSE SELECT * FROM patest0 join (SELECT id FROM
inttable LIMIT 1) ss ON patest0.id = ss.id;
QUERY PLAN
-------------------------------------------------------------------------------------------------
Nested Loop (cost=0.00..478.36 rows=2 width=12)
Output: patest0.id, patest0.x, inttable.id
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: inttable.id
-> Seq Scan on public.inttable (cost=0.00..145.00 rows=10000
width=4)
Output: inttable.id
-> Append (cost=0.00..478.31 rows=3 width=8)
-> Seq Scan on public.patest0 (cost=0.00..0.00 rows=1 width=8)
Output: patest0.id, patest0.x
Filter: (inttable.id = patest0.id)
-> Index Scan using patest1_id_idx on public.patest1
(cost=0.29..8.30 rows=1 width=8)
Output: patest1.id, patest1.x
Index Cond: (patest1.id = inttable.id)
-> Foreign Scan on public.patest2 (cost=100.00..470.00
rows=1 width=8)
Output: patest2.id, patest2.x
Remote SQL: SELECT id, x FROM public.mytable WHERE
(($1::integer = id))
Planning time: 0.233 ms
(17 rows)

I revised the patch. Patche attached, though I plan to update the
documentation further early next week.

Thanks,

Best regards,
Etsuro Fujita

(2014/03/20 21:59), Etsuro Fujita wrote:
> Here is a simple example for the case where the
> use_remote_estimate option is true:

Sorry, I incorrectly wrote it. The following example is for the case
where the option is *false*, as you see.

> # On mydatabase
>
> mydatabase=# CREATE TABLE mytable (id INTEGER, x INTEGER);
> CREATE TABLE
> mydatabase=# INSERT INTO mytable SELECT x, x FROM generate_series(0,
> 9999) x;
> INSERT 0 10000
>
> # On postgres
>
> postgres=# CREATE TABLE inttable (id INTEGER);
> CREATE TABLE
> postgres=# INSERT INTO inttable SELECT x FROM generate_series(0, 9999) x;
> INSERT 0 10000
> postgres=# ANALYZE inttable;
> ANALYZE
>
> postgres=# CREATE TABLE patest0 (id INTEGER, x INTEGER);
> CREATE TABLE
> postgres=# CREATE TABLE patest1 () INHERITS (patest0);
> CREATE TABLE
> postgres=# INSERT INTO patest1 SELECT x, x FROM generate_series(0, 9999) x;
> INSERT 0 10000
> postgres=# CREATE INDEX patest1_id_idx ON patest1(id);
> CREATE INDEX
> postgres=# CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (host 'localhost', dbname 'mydatabase');
> CREATE SERVER
> postgres=# CREATE USER MAPPING FOR PUBLIC SERVER myserver OPTIONS (user
> 'pgsql');
> CREATE USER MAPPING
> postgres=# CREATE FOREIGN TABLE patest2 () INHERITS (patest0) SERVER
> myserver OPTIONS (table_name 'mytable');
> CREATE FOREIGN TABLE
> postgres=# ANALYZE patest0;
> ANALYZE
> postgres=# ANALYZE patest1;
> ANALYZE
> postgres=# ANALYZE patest2;
> ANALYZE
> postgres=# EXPLAIN VERBOSE SELECT * FROM patest0 join (SELECT id FROM
> inttable LIMIT 1) ss ON patest0.id = ss.id;
> QUERY PLAN
> -------------------------------------------------------------------------------------------------
>
> Nested Loop (cost=0.00..478.36 rows=2 width=12)
> Output: patest0.id, patest0.x, inttable.id
> -> Limit (cost=0.00..0.01 rows=1 width=4)
> Output: inttable.id
> -> Seq Scan on public.inttable (cost=0.00..145.00 rows=10000
> width=4)
> Output: inttable.id
> -> Append (cost=0.00..478.31 rows=3 width=8)
> -> Seq Scan on public.patest0 (cost=0.00..0.00 rows=1 width=8)
> Output: patest0.id, patest0.x
> Filter: (inttable.id = patest0.id)
> -> Index Scan using patest1_id_idx on public.patest1
> (cost=0.29..8.30 rows=1 width=8)
> Output: patest1.id, patest1.x
> Index Cond: (patest1.id = inttable.id)
> -> Foreign Scan on public.patest2 (cost=100.00..470.00
> rows=1 width=8)
> Output: patest2.id, patest2.x
> Remote SQL: SELECT id, x FROM public.mytable WHERE
> (($1::integer = id))
> Planning time: 0.233 ms
> (17 rows)

Sorry for the delay.

Best regards,
Etsuro Fujita

(2014/03/20 21:59), Etsuro Fujita wrote:
> I revised the patch. Patche attached, though I plan to update the
> documentation further early next week.

I updated the documentation further and revise the patch further.
Attached is an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita

Hello, I could see reparameterize for foreign path to work
effectively thanks to your advice. The key point was setting
use_remote_estimate to false and existence of another child to
get parameterized path in prior stages.

The overall patch was applied on HEAD and compiled cleanly except
for a warning.

> analyze.c: In function ‘acquire_inherited_sample_rows’:
> analyze.c:1461: warning: unused variable ‘saved_rel’

As for postgres-fdw, the point I felt uneasy in previous patch
was fixed already:) - which was coping with omission of
ReparameterizeForeignPath.

And for file-fdw, you made a change to re-create foreignscan node
instead of the previous copy-and-modify. Is the reason you did it
that you considered the cost of 're-checking whether to
selectively perform binary conversion' is low enough? Or other
reasons?

Finally, although I insist the necessity of the warning for child
foreign tables on alter table, if you belive it to be put off,
I'll compromise by putting a note to CF-app that last judgement
is left to committer.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Horiguchi-san,

(2014/03/26 17:14), Kyotaro HORIGUCHI wrote:
> The overall patch was applied on HEAD and compiled cleanly except
> for a warning.
>
>> analyze.c: In function ‘acquire_inherited_sample_rows’:
>> analyze.c:1461: warning: unused variable ‘saved_rel’

I've fixed this in the latest version (v8) of the patch.

> And for file-fdw, you made a change to re-create foreignscan node
> instead of the previous copy-and-modify. Is the reason you did it
> that you considered the cost of 're-checking whether to
> selectively perform binary conversion' is low enough? Or other
> reasons?

The reason is that we get the result of the recheck from
path->fdw_private. Sorry, I'd forgotten it. So, I modified the code to
simply call create_foreignscan_path().

> Finally, although I insist the necessity of the warning for child
> foreign tables on alter table, if you belive it to be put off,
> I'll compromise by putting a note to CF-app that last judgement
> is left to committer.

OK So, if there are no objections of other, I'll mark this patch as
"ready for committer" and do that.

Thanks,

Best regards,
Etsuro Fujita

Hello,

> >> analyze.c: In function ‘acquire_inherited_sample_rows’:
> >> analyze.c:1461: warning: unused variable ‘saved_rel’
>
> I've fixed this in the latest version (v8) of the patch.

Mmm. sorry. I missed v8 patch. Then I had a look on that and have
a question.

You've added a check for relkind of baserel of the foreign path
to be reparameterized. If this should be an assertion - not a
conditional branch -, it would be better to put the assertion in
create_foreignscan_path instead of there.

=====
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1723,6 +1723,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
List *fdw_private)
{
ForeignPath *pathnode = makeNode(ForeignPath);
+ Assert(rel->rtekind == RTE_RELATION);

pathnode->path.pathtype = T_ForeignScan;
pathnode->path.parent = rel;
=====

> > And for file-fdw, you made a change to re-create foreignscan node
> > instead of the previous copy-and-modify. Is the reason you did it
> > that you considered the cost of 're-checking whether to
> > selectively perform binary conversion' is low enough? Or other
> > reasons?
>
> The reason is that we get the result of the recheck from
> path->fdw_private. Sorry, I'd forgotten it. So, I modified the code to
> simply call create_foreignscan_path().

Anyway you new code seems closer to the basics and the gain by
the previous optimization don't seem to be significant..

> > Finally, although I insist the necessity of the warning for child
> > foreign tables on alter table, if you belive it to be put off,
> > I'll compromise by putting a note to CF-app that last judgement
> > is left to committer.
>
> OK So, if there are no objections of other, I'll mark this patch as
> "ready for committer" and do that.

Thank you.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Horiguchi-san,

(2014/03/27 17:09), Kyotaro HORIGUCHI wrote:
>>>> analyze.c: In function ‘acquire_inherited_sample_rows’:
>>>> analyze.c:1461: warning: unused variable ‘saved_rel’
>>
>> I've fixed this in the latest version (v8) of the patch.
>
> Mmm. sorry. I missed v8 patch. Then I had a look on that and have
> a question.

Thank you for the review!

> You've added a check for relkind of baserel of the foreign path
> to be reparameterized. If this should be an assertion - not a
> conditional branch -, it would be better to put the assertion in
> create_foreignscan_path instead of there.
>
> =====
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -1723,6 +1723,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel,
> List *fdw_private)
> {
> ForeignPath *pathnode = makeNode(ForeignPath);
> + Assert(rel->rtekind == RTE_RELATION);
>
> pathnode->path.pathtype = T_ForeignScan;
> pathnode->path.parent = rel;
> =====

Maybe I'm missing the point, but I don't think it'd be better to put the
assertion in create_foreignscan_path(). And I think it'd be the caller'
responsiblity to ensure that equality, as any other pathnode creation
routine for a baserel in pathnode.c assumes that equality.

>>> And for file-fdw, you made a change to re-create foreignscan node
>>> instead of the previous copy-and-modify. Is the reason you did it
>>> that you considered the cost of 're-checking whether to
>>> selectively perform binary conversion' is low enough? Or other
>>> reasons?
>>
>> The reason is that we get the result of the recheck from
>> path->fdw_private. Sorry, I'd forgotten it. So, I modified the code to
>> simply call create_foreignscan_path().
>
> Anyway you new code seems closer to the basics and the gain by
> the previous optimization don't seem to be significant..

Yeah, that's true. I have to admit that the previous optimization is
nonsense.

Thanks,

Best regards,
Etsuro Fujita

(2014/03/27 10:49), Etsuro Fujita wrote:
> (2014/03/26 17:14), Kyotaro HORIGUCHI wrote:
>> Finally, although I insist the necessity of the warning for child
>> foreign tables on alter table, if you belive it to be put off,
>> I'll compromise by putting a note to CF-app that last judgement
>> is left to committer.
>
> OK So, if there are no objections of other, I'll mark this patch as
> "ready for committer" and do that.

I'll do it right now, since there seems to be no objections of others.

I've revised the patch a bit further, mainly the documentation. Patch
attached.

Note: as mentioned above, Horiguchi-san insisted that warning
functionality for recursive ALTER TABLE operations for inheritance trees
that contain one or more children that are foreign. However, that
functionality hasn't been included in the patch, since ISTM that that
functionality should be implemented as a separate patch [1]. We leave
this for commiters to decide.

[1] http://www.postgresql.org/message-id/5321ABD2.6000104@lab.ntt.co.jp

Thanks,

Best regards,
Etsuro Fujita

Hi,

> > ForeignPath *pathnode = makeNode(ForeignPath);
> > + Assert(rel->rtekind == RTE_RELATION);
> >
> > pathnode->path.pathtype = T_ForeignScan;
..
> Maybe I'm missing the point, but I don't think it'd be better to put the
> assertion in create_foreignscan_path(). And I think it'd be the caller'
> responsiblity to ensure that equality, as any other pathnode creation
> routine for a baserel in pathnode.c assumes that equality.

Hmm. The assertion (not shown above but you put in
parameterize_path:) seems to say that 'base relation for foreign
paths must be a RTE_RELATION' isn't right? But I don't see
anything putting such a restriction in reparameterize_path
itself. Could you tell me where such a restriction comes from? Or
who needs such a restriction? I think any assertions shouldn't be
anywhere other than where just before needed.

Thoughts?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/03/28 13:28), Kyotaro HORIGUCHI wrote:
> Hmm. The assertion (not shown above but you put in
> parameterize_path:) seems to say that 'base relation for foreign
> paths must be a RTE_RELATION' isn't right?

I think that's right. Please see allpaths.c, especially set_rel_pathlist().

For your information, the same assertion can be found in
create_foreignscan_plan().

Thanks,

Best regards,
Etsuro Fujita

I've found some bugs. Attached is an updated version (v10).

Changes:

* When CREATE FOREIGN TABLE INHERITS, don't allow a foreign table to
inherit from parent tables that have OID system columns.

* When CREATE FOREIGN TABLE INHERITS, reset the storage parameter for
inherited columns that have non-defaut values.

* Don't allow CHECK (expr) *NO INHERIT* on foreign tables, since those
tables cannot have child tables.

* Fix and update the documentation.

Thanks,

Best regards,
Etsuro Fujita

Attached is v11.

Changes:

* Rebased to head
* Improve an error message added to tablecmd.c to match it to existing
ones there
* Improve the documentaion a bit

Thanks,

Best regards,
Etsuro Fujita

(2014/04/02 21:25), Etsuro Fujita wrote:
> Attached is v11.
>
> Changes:
>
> * Rebased to head
> * Improve an error message added to tablecmd.c to match it to existing
> ones there
> * Improve the documentaion a bit

I moved this to 2014-06.

Since I've merged with the initial patch by Hanada-san (1) a feature to
allow the inherited stats to be computed by the ANALYZE command and (2)
a new FDW routine for path reparameterization, I put my name on the author.

Thanks,

Best regards,
Etsuro Fujita

Hello,

Before continueing discussion, I tried this patch on the current
head.

This patch applies cleanly except one hunk because of a
modification just AFTER it, which did no harm. Finally all
regression tests suceeded.

Attached is the rebased patch of v11 up to the current master.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,
Selecting tableoid on parent causes an error, "ERROR: cannot extract
system attribute from virtual tuple". The foreign table has an OID which
can be reported as tableoid for the rows coming from that foreign table. Do
we want to do that?

On Fri, Jun 20, 2014 at 1:34 PM, Kyotaro HORIGUCHI <
horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

> Hello,
>
> Before continueing discussion, I tried this patch on the current
> head.
>
> This patch applies cleanly except one hunk because of a
> modification just AFTER it, which did no harm. Finally all
> regression tests suceeded.
>
> Attached is the rebased patch of v11 up to the current master.
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi Ashutosh,

Thank you for the review.

(2014/06/23 18:35), Ashutosh Bapat wrote:
> Hi,
> Selecting tableoid on parent causes an error, "ERROR: cannot extract
> system attribute from virtual tuple". The foreign table has an OID which
> can be reported as tableoid for the rows coming from that foreign table.
> Do we want to do that?

No. I think it's a bug. I'll fix it.

Thanks,

Best regards,
Etsuro Fujita

(2014/06/24 16:30), Etsuro Fujita wrote:
> (2014/06/23 18:35), Ashutosh Bapat wrote:

>> Selecting tableoid on parent causes an error, "ERROR: cannot extract
>> system attribute from virtual tuple". The foreign table has an OID which
>> can be reported as tableoid for the rows coming from that foreign table.
>> Do we want to do that?
>
> No. I think it's a bug. I'll fix it.

Done. I think this is because create_foreignscan_plan() makes reference
to attr_needed, which isn't computed for inheritance children. To aboid
this, I've modified create_foreignscan_plan() to see reltargetlist and
baserestrictinfo, instead of attr_needed. Please find attached an
updated version of the patch.

Sorry for the delay.

Best regards,
Etsuro Fujita

I checked that it's reporting the right tableoid now.

BTW, why aren't you using the tlist passed to this function? I guess
create_scan_plan() passes tlist after processing it, so that should be used
rather than rel->reltargetlist.

On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> (2014/06/24 16:30), Etsuro Fujita wrote:
>
>> (2014/06/23 18:35), Ashutosh Bapat wrote:
>>
>
> Selecting tableoid on parent causes an error, "ERROR: cannot extract
>>> system attribute from virtual tuple". The foreign table has an OID which
>>> can be reported as tableoid for the rows coming from that foreign table.
>>> Do we want to do that?
>>>
>>
>> No. I think it's a bug. I'll fix it.
>>
>
> Done. I think this is because create_foreignscan_plan() makes reference
> to attr_needed, which isn't computed for inheritance children. To aboid
> this, I've modified create_foreignscan_plan() to see reltargetlist and
> baserestrictinfo, instead of attr_needed. Please find attached an updated
> version of the patch.
>
>
> Sorry for the delay.
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/06/30 17:47), Ashutosh Bapat wrote:
> I checked that it's reporting the right tableoid now.

Thank you for the check.

> BTW, why aren't you using the tlist passed to this function? I guess
> create_scan_plan() passes tlist after processing it, so that should be
> used rather than rel->reltargetlist.

I think that that would be maybe OK, but I think that it would not be
efficient to use the list to compute attrs_used, because the tlist would
have more information than rel->reltargetlist in cases where the tlist
is build through build_physical_tlist().

Thanks,

Best regards,
Etsuro Fujita

>
> On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>
> (2014/06/24 16:30), Etsuro Fujita wrote:
>
> (2014/06/23 18:35), Ashutosh Bapat wrote:
>
>
> Selecting tableoid on parent causes an error, "ERROR:
> cannot extract
> system attribute from virtual tuple". The foreign table has
> an OID which
> can be reported as tableoid for the rows coming from that
> foreign table.
> Do we want to do that?
>
>
> No. I think it's a bug. I'll fix it.
>
>
> Done. I think this is because create_foreignscan_plan() makes
> reference to attr_needed, which isn't computed for inheritance
> children. To aboid this, I've modified create_foreignscan_plan() to
> see reltargetlist and baserestrictinfo, instead of attr_needed.
> Please find attached an updated version of the patch.
>
>
> Sorry for the delay.
>
> Best regards,
> Etsuro Fujita
>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company

On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/06/30 17:47), Ashutosh Bapat wrote:
>
>> I checked that it's reporting the right tableoid now.
>>
>
> Thank you for the check.
>
>
> BTW, why aren't you using the tlist passed to this function? I guess
>> create_scan_plan() passes tlist after processing it, so that should be
>> used rather than rel->reltargetlist.
>>
>
> I think that that would be maybe OK, but I think that it would not be
> efficient to use the list to compute attrs_used, because the tlist would
> have more information than rel->reltargetlist in cases where the tlist is
> build through build_physical_tlist().
>
>
In that case, we can call build_relation_tlist() for foreign tables.

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>
>
>> On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>>
>> (2014/06/24 16:30), Etsuro Fujita wrote:
>>
>> (2014/06/23 18:35), Ashutosh Bapat wrote:
>>
>>
>> Selecting tableoid on parent causes an error, "ERROR:
>> cannot extract
>> system attribute from virtual tuple". The foreign table has
>> an OID which
>> can be reported as tableoid for the rows coming from that
>> foreign table.
>> Do we want to do that?
>>
>>
>> No. I think it's a bug. I'll fix it.
>>
>>
>> Done. I think this is because create_foreignscan_plan() makes
>> reference to attr_needed, which isn't computed for inheritance
>> children. To aboid this, I've modified create_foreignscan_plan() to
>> see reltargetlist and baserestrictinfo, instead of attr_needed.
>> Please find attached an updated version of the patch.
>>
>>
>> Sorry for the delay.
>>
>> Best regards,
>> Etsuro Fujita
>>
>>
>>
>>
>> --
>> Best Wishes,
>> Ashutosh Bapat
>> EnterpriseDB Corporation
>> The Postgres Database Company
>>
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> Done. I think this is because create_foreignscan_plan() makes reference
> to attr_needed, which isn't computed for inheritance children.

I wonder whether it isn't time to change that. It was coded like that
originally only because calculating the values would've been a waste of
cycles at the time. But this is at least the third place where it'd be
useful to have attr_needed for child rels.

regards, tom lane

(2014/06/30 20:17), Ashutosh Bapat wrote:
> On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:

> (2014/06/30 17:47), Ashutosh Bapat wrote:

> BTW, why aren't you using the tlist passed to this function? I guess
> create_scan_plan() passes tlist after processing it, so that
> should be
> used rather than rel->reltargetlist.

> I think that that would be maybe OK, but I think that it would not
> be efficient to use the list to compute attrs_used, because the
> tlist would have more information than rel->reltargetlist in cases
> where the tlist is build through build_physical_tlist().

> In that case, we can call build_relation_tlist() for foreign tables.

Do you mean build_physical_tlist()?

Yeah, we can call build_physical_tlist() (and do that in some cases),
but if we call the function, it would generate a tlist that contains all
Vars in the relation, not only those Vars actually needed by the query
(ie, Vars in reltargetlist), and thus it would take more cycles to
compute attr_used from the tlist than from reltargetlist. That' what I
wanted to say.

Thanks,

Best regards,
Etsuro Fujita

(2014/06/30 22:48), Tom Lane wrote:
> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> Done. I think this is because create_foreignscan_plan() makes reference
>> to attr_needed, which isn't computed for inheritance children.
>
> I wonder whether it isn't time to change that. It was coded like that
> originally only because calculating the values would've been a waste of
> cycles at the time. But this is at least the third place where it'd be
> useful to have attr_needed for child rels.

+1 for calculating attr_needed for child rels. (I was wondering too.)

I'll create a separate patch for it.

Thanks,

Best regards,
Etsuro Fujita

Hello,

> > BTW, why aren't you using the tlist passed to this function? I guess
> >> create_scan_plan() passes tlist after processing it, so that should be
> >> used rather than rel->reltargetlist.
> >>
> >
> > I think that that would be maybe OK, but I think that it would not be
> > efficient to use the list to compute attrs_used, because the tlist would
> > have more information than rel->reltargetlist in cases where the tlist is
> > build through build_physical_tlist().
> >
> >
> In that case, we can call build_relation_tlist() for foreign tables.

# is it build_base_rel_tilst() ?

It needs only one effective line added, which would be seemingly
far simple ingoring potential extra calculation for non-child
relations (I suppose getting rid of it needs a foreach on
rel->append_rel_list..) and too-longer physical tlist. On the
other hand build_relation_tlist() and this patch seem to be in
the same order of computational complexity for the length of the
tlist.

I prefer to use build_base_rel_tlist() for the clarity of
code. But I don't know how high the chance to big physical tlist
and non-child relations become to be an annoyance. Is there any
suggestions?

By the way, I tried xmin and xmax for the file_fdw tables.

postgres=# select tableoid, xmin, xmax, * from passwd1;
tableoid | xmin | xmax | uname | pass | uid | gid | ..
16396 | 244 | 4294967295 | root | x | 0 | 0 | root...
16396 | 252 | 4294967295 | bin | x | 1 | 1 | bin...
16396 | 284 | 4294967295 | daemon | x | 2 | 2 | daemon...

The xmin and xmax apparently doesn't look sane. After some
investigation, I found that they came from the following place in
heap_form_tuple(), (call stach is show below)

| HeapTupleHeaderSetDatumLength(td, len);
| HeapTupleHeaderSetTypeId(td, tupleDescriptor->tdtypeid);
| HeapTupleHeaderSetTypMod(td, tupleDescriptor->tdtypmod);

HeapTupleHeader is a union of HeapTupleFields and
DatumTupleFields and these macors seem to treat it as the
latter. Then later this tuple seems to be read as the former so
xmin and xmax should have that values.

This seems to be a bug. But I have no idea how to deal with it
for now. I'll take more look into this.

The call stack onto the above heap_form_tuple is as follows.

#0 heap_form_tuple (tupleDescriptor=0x7fc46d2953a8, values=0x10dd1a0, isnull=0x10dd1f8 "") at heaptuple.c:731
#1 ExecCopySlotTuple (slot=0x10dd0e0) at execTuples.c:574
#2 ExecMaterializeSlot (slot=0x10dd0e0) at execTuples.c:760
#3 ForeignNext (node=0x10dc7b0) at nodeForeignscan.c:61
#4 ExecScanFetch (node=0x10dc7b0, accessMtd=0x66599c <ForeignNext>, recheckMtd=0x665a43 <ForeignRecheck>) at execScan.c:82
#5 ExecScan (node=0x10dc7b0, accessMtd=0x66599c <ForeignNext>, recheckMtd=0x665a43 <ForeignRecheck>) at execScan.c:167
#6 ExecForeignScan (node=0x10dc7b0) at nodeForeignscan.c:91
#7 ExecProcNode (node=0x10dc7b0) at execProcnode.c:442

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

If we are going to change that portion of the code, we may as well go a bit
forward and allow any expressions to be fetched from a foreign server
(obviously, if that server is capable of doing so). It will help, when we
come to aggregate push-down or whole query push-down (whenever that
happens). So, instead of attr_needed, which restricts only the attributes
to be fetched, why not to targetlist itself?

On Mon, Jun 30, 2014 at 7:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > Done. I think this is because create_foreignscan_plan() makes reference
> > to attr_needed, which isn't computed for inheritance children.
>
> I wonder whether it isn't time to change that. It was coded like that
> originally only because calculating the values would've been a waste of
> cycles at the time. But this is at least the third place where it'd be
> useful to have attr_needed for child rels.
>
> regards, tom lane
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/06/30 20:17), Ashutosh Bapat wrote:
>
>> On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>>
>
> (2014/06/30 17:47), Ashutosh Bapat wrote:
>>
>
> BTW, why aren't you using the tlist passed to this function? I
>> guess
>> create_scan_plan() passes tlist after processing it, so that
>> should be
>> used rather than rel->reltargetlist.
>>
>
> I think that that would be maybe OK, but I think that it would not
>> be efficient to use the list to compute attrs_used, because the
>> tlist would have more information than rel->reltargetlist in cases
>> where the tlist is build through build_physical_tlist().
>>
>
> In that case, we can call build_relation_tlist() for foreign tables.
>>
>
> Do you mean build_physical_tlist()?
>
>
Sorry, I meant build_path_tlist() or disuse_physical_tlist() whichever is
appropriate.

We may want to modify use_physical_tlist(), to return false, in case of
foreign tables. BTW, it does return false for inheritance trees.

486 /*
487 * Can't do it with inheritance cases either (mainly because Append
488 * doesn't project).
489 */
490 if (rel->reloptkind != RELOPT_BASEREL)
491 return false;

Yeah, we can call build_physical_tlist() (and do that in some cases), but
> if we call the function, it would generate a tlist that contains all Vars
> in the relation, not only those Vars actually needed by the query (ie, Vars
> in reltargetlist), and thus it would take more cycles to compute attr_used
> from the tlist than from reltargetlist. That' what I wanted to say.
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hello,

Sorry, this was no relation with this patch.

ForeignNext materializes the slot, which would be any of physical
and virtual tuple, when system column was requested. If it was a
virtual one, file_fdw makes this, heap_form_tuple generates the
tuple as DatumTuple. The result is a jumble of virtual and
physical. But the returning slot has tts_tuple so the caller
interprets it as a physical one.

Anyway the requests for xmin/xmax could not be prevented
beforehand in current framework, I did rewrite the physical tuple
header so as to really be a physical one.

This would be another patch, so I will put this into next CF if
this don't get any immediate objection.

> By the way, I tried xmin and xmax for the file_fdw tables.
>
> postgres=# select tableoid, xmin, xmax, * from passwd1;
> tableoid | xmin | xmax | uname | pass | uid | gid | ..
> 16396 | 244 | 4294967295 | root | x | 0 | 0 | root...
> 16396 | 252 | 4294967295 | bin | x | 1 | 1 | bin...
> 16396 | 284 | 4294967295 | daemon | x | 2 | 2 | daemon...
>
> The xmin and xmax apparently doesn't look sane. After some
> investigation, I found that they came from the following place in
> heap_form_tuple(), (call stach is show below)

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/07/01 15:13), Ashutosh Bapat wrote:
> On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:

> We may want to modify use_physical_tlist(), to return false, in case of
> foreign tables. BTW, it does return false for inheritance trees.

Yeah, but please consider cases where foreign tables are not inheritance
child rels (and any system columns are requested).

> 486 /*
> 487 * Can't do it with inheritance cases either (mainly because
> Append
> 488 * doesn't project).
> 489 */
> 490 if (rel->reloptkind != RELOPT_BASEREL)
> 491 return false;
>
> Yeah, we can call build_physical_tlist() (and do that in some
> cases), but if we call the function, it would generate a tlist that
> contains all Vars in the relation, not only those Vars actually
> needed by the query (ie, Vars in reltargetlist), and thus it would
> take more cycles to compute attr_used from the tlist than from
> reltargetlist. That' what I wanted to say.

Maybe I'm missing something, but what's the point of using the tlist,
not reltargetlist?

Thanks,

Best regards,
Etsuro Fujita

On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/07/01 15:13), Ashutosh Bapat wrote:
>
>> On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>>
>
> We may want to modify use_physical_tlist(), to return false, in case of
>> foreign tables. BTW, it does return false for inheritance trees.
>>
>
> Yeah, but please consider cases where foreign tables are not inheritance
> child rels (and any system columns are requested).
>
>
> 486 /*
>> 487 * Can't do it with inheritance cases either (mainly because
>> Append
>> 488 * doesn't project).
>> 489 */
>> 490 if (rel->reloptkind != RELOPT_BASEREL)
>> 491 return false;
>>
>> Yeah, we can call build_physical_tlist() (and do that in some
>> cases), but if we call the function, it would generate a tlist that
>> contains all Vars in the relation, not only those Vars actually
>> needed by the query (ie, Vars in reltargetlist), and thus it would
>> take more cycles to compute attr_used from the tlist than from
>> reltargetlist. That' what I wanted to say.
>>
>
> Maybe I'm missing something, but what's the point of using the tlist, not
> reltargetlist?
>
>
Compliance with other create_*scan_plan() functions. The tlist passed to
those functions is sometimes preprocessed in create_scan_plan() and some of
the function it calls. If we use reltargetlist directly, we loose that
preprocessing. I have not see any of create_*scan_plan() fetch the
targetlist directly from RelOptInfo. It is always the one supplied by
build_path_tlist() or disuse_physical_tlist() (which in turn calls
build_path_tlist()) or build_physical_tlist().

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/07/01 16:04), Ashutosh Bapat wrote:
> On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:

> Maybe I'm missing something, but what's the point of using the
> tlist, not reltargetlist?

> Compliance with other create_*scan_plan() functions. The tlist passed to
> those functions is sometimes preprocessed in create_scan_plan() and some
> of the function it calls. If we use reltargetlist directly, we loose
> that preprocessing. I have not see any of create_*scan_plan() fetch the
> targetlist directly from RelOptInfo. It is always the one supplied by
> build_path_tlist() or disuse_physical_tlist() (which in turn calls
> build_path_tlist()) or build_physical_tlist().

I've got the point.

As I said upthread, I'll work on calculating attr_needed for child rels,
and I hope that that will eliminate your concern.

Thanks,

Best regards,
Etsuro Fujita

Hi,

At Tue, 01 Jul 2014 16:30:41 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <53B263A1(dot)3060107(at)lab(dot)ntt(dot)co(dot)jp>
> I've got the point.
>
> As I said upthread, I'll work on calculating attr_needed for child
> rels, and I hope that that will eliminate your concern.

Inheritance tree is expanded far after where attr_needed for the
parent built, in set_base_rel_sizes() in make_one_rel(). I'm
afraid that building all attr_needed for whole inheritance tree
altogether is a bit suffering.

I have wanted the point of inheritance expansion earlier for
another patch. Do you think that rearranging there? Or generate
them individually in crete_foreign_plan()?

Anyway, I'm lookin forward to your next patch. So no answer
needed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:
> Attached is the rebased patch of v11 up to the current master.

I've been studying this patch.

SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error
condition, even when SELECT FOR UPDATE on the child foreign table alone would
have succeeded.

The patch adds zero tests. Add principal tests to postgres_fdw.sql. Also,
create a child foreign table in foreign_data.sql; this will make dump/reload
tests of the regression database exercise an inheritance tree that includes a
foreign table.

The inheritance section of ddl.sgml should mention child foreign tables, at
least briefly. Consider mentioning it in the partitioning section, too.

Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:

INFO: analyzing "test_foreign_inherit.parent"
INFO: "parent": scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 1 rows in sample, 1 estimated total rows
INFO: analyzing "test_foreign_inherit.parent" inheritance tree
WARNING: relcache reference leak: relation "child" not closed
WARNING: relcache reference leak: relation "tchild" not closed
WARNING: relcache reference leak: relation "parent" not closed

Please arrange to omit the 'analyzing "tablename" inheritance tree' message,
since this ANALYZE actually skipped that task. (The warnings obviously need a
fix, too.) I do find it awkward that adding a foreign table to an inheritance
tree will make autovacuum stop collecting statistics on that inheritance tree,
but I can't think of a better policy.

The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.

We use the term "child table" in many messages. Should that change to
something more inclusive, now that the child may be a foreign table? Perhaps
one of "child relation", plain "child", or "child foreign table"/"child table"
depending on the actual object? "child relation" is robust technically, but
it might add more confusion than it removes. Varying the message depending on
the actual object feels like a waste. Opinions?

LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK
TABLE fails when given a foreign table directly. That's odd, but I see no
cause to change it.

A partition root only accepts an UPDATE command if every child is updatable.
Similarly, "UPDATE ... WHERE CURRENT OF cursor_name" fails if any child does
not support it. That seems fine. Incidentally, does anyone have a FDW that
supports WHERE CURRENT OF?

The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns
to match the order found in parents. That is, both of these tables actually
have columns in the order (a,b):

create table parent (a int, b int);
create table child (b int, a int) inherits (parent);

Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing
column order in this way. (pg_dump --binary-upgrade uses ALTER TABLE INHERIT
and some catalog hacks to avoid doing so.) I've never liked that dump/reload
can change column order, but it's tolerable if you follow the best practice of
always writing out column lists. The stakes rise for foreign tables, because
column order is inherently significant to file_fdw and probably to certain
other non-RDBMS FDWs. If pg_dump changes column order in your file_fdw
foreign table, the table breaks. I would heartily support making pg_dump
preserve column order for all inheritance children. That doesn't rise to the
level of being a blocker for this patch, though.

I am attaching rough-hewn tests I used during this review.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Hi Fujita-san,

Sorry for leaving this thread for long time.

2014-06-24 16:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/06/23 18:35), Ashutosh Bapat wrote:
>>
>> Hi,
>> Selecting tableoid on parent causes an error, "ERROR: cannot extract
>> system attribute from virtual tuple". The foreign table has an OID which
>> can be reported as tableoid for the rows coming from that foreign table.
>> Do we want to do that?
>
>
> No. I think it's a bug. I'll fix it.

IIUC, you mean that tableoid can't be retrieved when a foreign table
is accessed via parent table, but it sounds strange to me, because one
of main purposes of tableoid is determine actual source table in
appended results.

Am I missing the point?

--
Shigeru HANADA

(2014/07/10 18:12), Shigeru Hanada wrote:
> 2014-06-24 16:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> (2014/06/23 18:35), Ashutosh Bapat wrote:

>>> Selecting tableoid on parent causes an error, "ERROR: cannot extract
>>> system attribute from virtual tuple". The foreign table has an OID which
>>> can be reported as tableoid for the rows coming from that foreign table.
>>> Do we want to do that?

>> No. I think it's a bug. I'll fix it.

> IIUC, you mean that tableoid can't be retrieved when a foreign table
> is accessed via parent table,

No. What I want to say is that tableoid *can* be retrieved when a
foreign table is accessed via the parent table.

Thanks,

Best regards,
Etsuro Fujita

(2014/07/11 15:50), Etsuro Fujita wrote:
> (2014/07/10 18:12), Shigeru Hanada wrote:

>> IIUC, you mean that tableoid can't be retrieved when a foreign table
>> is accessed via parent table,
>
> No. What I want to say is that tableoid *can* be retrieved when a
> foreign table is accessed via the parent table.

In fact, you can do that with v13 [1], but I plan to change the way of
fixing (see [2]).

Thanks,

[1] http://www.postgresql.org/message-id/53B10914.2010504@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/53B2188B.4090302@lab.ntt.co.jp

Best regards,
Etsuro Fujita

Hello, I found that tuples come from file_fdw has strange xmin and xmax.

> postgres=# select tableoid, xmin, xmax, * from passwd1;
> tableoid | xmin | xmax | uname | pass | uid | gid | ..
> 16396 | 244 | 4294967295 | root | x | 0 | 0 | root...
> 16396 | 252 | 4294967295 | bin | x | 1 | 1 | bin...
> 16396 | 284 | 4294967295 | daemon | x | 2 | 2 | daemon...

Back to 9.1 gives the same result.

These xmin and xmax are the simple interpretations of a
DatumTupleFields filled by ExecMaterializedSlot() beeing fed the
source virtual tuple slot from file_fdw. On the other hand,
postgres_fdw gives sane xids (xmin = 2, xmax = 0).

This is because ForeignNext returns physical tuples in which
their headers are DatumTupleFields regardless whether the system
columns are requested or not. The fdw machinary desn't seem to
provide fdw handlers with a means to reject requests for
unavailable system columns, so the tuple header should be fixed
with the sane values as HeapTupleFields.

The patch attached fixes the header of materialized tuple to be
sane (2, 0) if the source slot was a virtual tuple in mechanism().

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> Hello, I found that tuples come from file_fdw has strange xmin and xmax.

file_fdw isn't documented to return anything useful for xmin/xmax/etc,
so I don't find this surprising.

> The patch attached fixes the header of materialized tuple to be
> sane (2, 0) if the source slot was a virtual tuple in mechanism().

I don't really think it's ForeignNext's place to be doing something
about this. It seems like a useless expenditure of cycles. Besides,
this fails to cover e.g. postgres_fdw, which is likewise unconcerned
about what it returns for system columns other than ctid.

regards, tom lane

Hello, sorry, it's my bad.

> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > Hello, I found that tuples come from file_fdw has strange xmin and xmax.
>
> file_fdw isn't documented to return anything useful for xmin/xmax/etc,
> so I don't find this surprising.
>
> > The patch attached fixes the header of materialized tuple to be
> > sane (2, 0) if the source slot was a virtual tuple in mechanism().
>
> I don't really think it's ForeignNext's place to be doing something
> about this. It seems like a useless expenditure of cycles. Besides,
> this fails to cover e.g. postgres_fdw, which is likewise unconcerned
> about what it returns for system columns other than ctid.

I somehow misunderstood that postgres_fdw returns (xmin, xmax) =
(2, 0) but I confirmed that xmin, xmax and citd seems insane.

I completely agree with you.

Thank you for giving response for the stupid story.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

(2014/07/01 11:10), Etsuro Fujita wrote:
> (2014/06/30 22:48), Tom Lane wrote:
>> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>>> Done. I think this is because create_foreignscan_plan() makes reference
>>> to attr_needed, which isn't computed for inheritance children.
>>
>> I wonder whether it isn't time to change that. It was coded like that
>> originally only because calculating the values would've been a waste of
>> cycles at the time. But this is at least the third place where it'd be
>> useful to have attr_needed for child rels.
>
> +1 for calculating attr_needed for child rels. (I was wondering too.)
>
> I'll create a separate patch for it.

Attached is a WIP patch for that. The following functions have been
changed to refer to attr_needed:

* check_index_only()
* remove_unused_subquery_outputs()
* check_selective_binary_conversion()

I'll add this to the upcoming commitfest. If anyone has any time to
glance at it before then, that would be a great help.

Thanks,

Best regards,
Etsuro Fujita

(2014/08/06 20:43), Etsuro Fujita wrote:
>> (2014/06/30 22:48), Tom Lane wrote:
>>> I wonder whether it isn't time to change that. It was coded like that
>>> originally only because calculating the values would've been a waste of
>>> cycles at the time. But this is at least the third place where it'd be
>>> useful to have attr_needed for child rels.

> Attached is a WIP patch for that.

I've revised the patch.

Changes:

* Make the code more readable
* Revise the comments
* Cleanup

Please find attached an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita

(2014/08/08 18:51), Etsuro Fujita wrote:
>>> (2014/06/30 22:48), Tom Lane wrote:
>>>> I wonder whether it isn't time to change that. It was coded like that
>>>> originally only because calculating the values would've been a waste of
>>>> cycles at the time. But this is at least the third place where it'd be
>>>> useful to have attr_needed for child rels.

> I've revised the patch.

There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.

The previous patch doesn't cope with some UNION ALL cases properly. So,
e.g., the server will crash for the following query:

postgres=# create table ta1 (f1 int);
CREATE TABLE
postgres=# create table ta2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# create table tb1 (f1 int);
CREATE TABLE
postgres=# create table tb2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;

With the updated version, we get the right result:

postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
QUERY PLAN
--------------------------------------------------------------------------------
Append (cost=0.00..0.05 rows=2 width=4)
-> Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4)
Output: ssa.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: ta1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400
width=4)
Output: ta1.f1, NULL::integer, NULL::integer
-> Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4)
Output: ssb.f1
-> Limit (cost=0.00..0.01 rows=1 width=4)
Output: tb1.f1, (NULL::integer), (NULL::integer)
-> Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400
width=4)
Output: tb1.f1, NULL::integer, NULL::integer
Planning time: 0.453 ms
(14 rows)

While thinking to address this problem, Ashutosh also expressed concern
about the UNION ALL handling in the previous patch in a private email.
Thank you for the review, Ashutosh!

Thanks,

Best regards,
Etsuro Fujita

Hi,

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> (2014/08/08 18:51), Etsuro Fujita wrote:
> >>> (2014/06/30 22:48), Tom Lane wrote:
> >>>> I wonder whether it isn't time to change that. It was coded like that
> >>>> originally only because calculating the values would've been a waste
> of
> >>>> cycles at the time. But this is at least the third place where it'd
> be
> >>>> useful to have attr_needed for child rels.
>
> > I've revised the patch.
>
> There was a problem with the previous patch, which will be described
> below. Attached is the updated version of the patch addressing that.
>
> The previous patch doesn't cope with some UNION ALL cases properly. So,
> e.g., the server will crash for the following query:
>
> postgres=# create table ta1 (f1 int);
> CREATE TABLE
> postgres=# create table ta2 (f2 int primary key, f3 int);
> CREATE TABLE
> postgres=# create table tb1 (f1 int);
> CREATE TABLE
> postgres=# create table tb2 (f2 int primary key, f3 int);
> CREATE TABLE
> postgres=# explain verbose select f1 from ((select f1, f2 from (select
> f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
> (select f1,
> f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
> ssb)) ss;
>
> With the updated version, we get the right result:
>
> postgres=# explain verbose select f1 from ((select f1, f2 from (select
> f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
> (select f1,
> f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
> ssb)) ss;
> QUERY PLAN
>
> --------------------------------------------------------------------------------
> Append (cost=0.00..0.05 rows=2 width=4)
> -> Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4)
> Output: ssa.f1
> -> Limit (cost=0.00..0.01 rows=1 width=4)
> Output: ta1.f1, (NULL::integer), (NULL::integer)
> -> Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400
> width=4)
> Output: ta1.f1, NULL::integer, NULL::integer
> -> Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4)
> Output: ssb.f1
> -> Limit (cost=0.00..0.01 rows=1 width=4)
> Output: tb1.f1, (NULL::integer), (NULL::integer)
> -> Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400
> width=4)
> Output: tb1.f1, NULL::integer, NULL::integer
> Planning time: 0.453 ms
> (14 rows)
>
> While thinking to address this problem, Ashutosh also expressed concern
> about the UNION ALL handling in the previous patch in a private email.
> Thank you for the review, Ashutosh!
>
>
Thanks for taking care of this one.

Here are some more comments

attr_needed also has the attributes used in the restriction clauses as seen
in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().

Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds
to FirstLowInvalidHeapAttributeNumber + 1, it's always safer to use it is
RelOptInfo::min_attr, in case get_relation_info() wants to change
assumption or somewhere down the line some other part of code wants to
change attr_needed information. It may be unlikely, that it would change,
but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often <0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed; /* array indexed [min_attr ..
max_attr] */

> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company