Re: inherit support for foreign tables

(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