Re: inherit support for foreign tables

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