[Review] inherit support for foreign tables

Shigeru Hanada wrote:
> Attached patch allows a foreign table to be a child of a table. It
> also allows foreign tables to have CHECK constraints.

Sorry for the delay. I started to look at this patch.

> 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.

I agree with you, but we should introduce a new keyword such as
ASSERTIVE or something in 9.4, in preparation for the enforcement of
constraints on the local side in a future release? What I'm concerned
about is, otherwise, users will have to rewrite those DDL queries at
that point. No?

> 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.

I agree with you on that point.

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

I'm not sure that in the processing of the ANALYZE command, we should
ignore child foreign tables. It seems to me that the recursive
processing is not that hard. Rather what I'm concerned about is that if
the recursive processing is allowed, then autovacuum will probably have
to access to forign tables on the far side in order to acquire those
sample rows. It might be undesirable from the viewpoint of security or
from the viewpoint of efficiency.

--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -22,6 +22,7 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable
class="PARAMETER">table_name
<replaceable class="PARAMETER">column_name</replaceable> <replaceable
class="PARAMETER">data_type</replaceable> [ OPTIONS ( <replaceable
class="PA\
RAMETER">option</replaceable> '<replaceable
class="PARAMETER">value</replaceable>' [, ... ] ) ] [ COLLATE
<replaceable>collation</replaceable> ] [ <rep\
laceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
[, ... ]
] )
+[ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
SERVER <replaceable class="parameter">server_name</replaceable>
[ OPTIONS ( <replaceable class="PARAMETER">option</replaceable>
'<replaceable class="PARAMETER">value</replaceable>' [, ... ] ) ]

@@ -159,6 +160,17 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable
class="PARAMETER">table_name
</varlistentry>

<varlistentry>
+ <term><replaceable class="PARAMETER">parent_table</replaceable></term>
+ <listitem>
+ <para>
+ The name of an existing table or foreign table from which the new foreign
+ table automatically inherits all columns, see
+ <xref linkend="ddl-inherit"> for details of table inheritance.
+ </para>
+ </listitem>
+ </varlistentry>

Correct? I think we should not allow a foreign table to be a parent.

I'll look at this patch more closely.

Thanks,

Best regards,
Etsuro Fujita

Hi Fujita-san,

Thanks for the review.

2014/1/23 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> Shigeru Hanada wrote:
>> 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.
>
> I agree with you, but we should introduce a new keyword such as
> ASSERTIVE or something in 9.4, in preparation for the enforcement of
> constraints on the local side in a future release? What I'm concerned
> about is, otherwise, users will have to rewrite those DDL queries at
> that point. No?

In the original thread, whether the new syntax is necessary (maybe
ASSERTIVE will not be used though) is under discussion. Anyway, your
point, decrease user's DDL rewrite less as possible is important.
Could you post review result and/or your opinion as the reply to the
original thread, then we include other people interested in this
feature can share discussion.

>> 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.
>
> I agree with you on that point.
>
>> For other commands recursively processed such as ANALYZE, foreign
>> tables in the leaf of inheritance tree are ignored.
>
> I'm not sure that in the processing of the ANALYZE command, we should
> ignore child foreign tables. It seems to me that the recursive
> processing is not that hard. Rather what I'm concerned about is that if
> the recursive processing is allowed, then autovacuum will probably have
> to access to forign tables on the far side in order to acquire those
> sample rows. It might be undesirable from the viewpoint of security or
> from the viewpoint of efficiency.

As you say, it's not difficult to do recursive ANALYZE including
foreign tables. The reason why ANALYZE ignores descendant foreign
tables is consistent behavior. At the moment, ANALYZE ignores foreign
tables in top-level (non-child) when no table name was given, and if
we want to ANALYZE foreign tables we need to specify explicitly.

I think it's not so bad to show WARNING or INFO message about foreign
table ignorance, including which is not a child.

> --- a/doc/src/sgml/ref/create_foreign_table.sgml
> +++ b/doc/src/sgml/ref/create_foreign_table.sgml
> @@ -22,6 +22,7 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable
> class="PARAMETER">table_name
> <replaceable class="PARAMETER">column_name</replaceable> <replaceable
> class="PARAMETER">data_type</replaceable> [ OPTIONS ( <replaceable
> class="PA\
> RAMETER">option</replaceable> '<replaceable
> class="PARAMETER">value</replaceable>' [, ... ] ) ] [ COLLATE
> <replaceable>collation</replaceable> ] [ <rep\
> laceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> [, ... ]
> ] )
> +[ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> SERVER <replaceable class="parameter">server_name</replaceable>
> [ OPTIONS ( <replaceable class="PARAMETER">option</replaceable>
> '<replaceable class="PARAMETER">value</replaceable>' [, ... ] ) ]
>
> @@ -159,6 +160,17 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable
> class="PARAMETER">table_name
> </varlistentry>
>
> <varlistentry>
> + <term><replaceable class="PARAMETER">parent_table</replaceable></term>
> + <listitem>
> + <para>
> + The name of an existing table or foreign table from which the new foreign
> + table automatically inherits all columns, see
> + <xref linkend="ddl-inherit"> for details of table inheritance.
> + </para>
> + </listitem>
> + </varlistentry>
>
> Correct? I think we should not allow a foreign table to be a parent.

Oops, good catch.
--
Shigeru HANADA

(2014/01/25 11:27), Shigeru Hanada wrote:
> 2014/1/23 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> Shigeru Hanada wrote:
>>> 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.
>>
>> I agree with you, but we should introduce a new keyword such as
>> ASSERTIVE or something in 9.4, in preparation for the enforcement of
>> constraints on the local side in a future release? What I'm concerned
>> about is, otherwise, users will have to rewrite those DDL queries at
>> that point. No?
>
> In the original thread, whether the new syntax is necessary (maybe
> ASSERTIVE will not be used though) is under discussion. Anyway, your
> point, decrease user's DDL rewrite less as possible is important.
> Could you post review result and/or your opinion as the reply to the
> original thread, then we include other people interested in this
> feature can share discussion.

OK I'll give my opinion in that thread.

>>> For other commands recursively processed such as ANALYZE, foreign
>>> tables in the leaf of inheritance tree are ignored.
>>
>> I'm not sure that in the processing of the ANALYZE command, we should
>> ignore child foreign tables. It seems to me that the recursive
>> processing is not that hard. Rather what I'm concerned about is that if
>> the recursive processing is allowed, then autovacuum will probably have
>> to access to forign tables on the far side in order to acquire those
>> sample rows. It might be undesirable from the viewpoint of security or
>> from the viewpoint of efficiency.
>
> As you say, it's not difficult to do recursive ANALYZE including
> foreign tables. The reason why ANALYZE ignores descendant foreign
> tables is consistent behavior. At the moment, ANALYZE ignores foreign
> tables in top-level (non-child) when no table name was given, and if
> we want to ANALYZE foreign tables we need to specify explicitly.
>
> I think it's not so bad to show WARNING or INFO message about foreign
> table ignorance, including which is not a child.

Yeah, the consistency is essential for its ease of use. But I'm not
sure that inherited stats ignoring foreign tables is actually useful for
query optimization. What I think about the consistency is a) the
ANALYZE command with no table names skips ANALYZEing inheritance trees
that include at least one foreign table as a child, but b) the ANALYZE
command with a table name indicating an inheritance tree that includes
any foreign tables does compute the inherited stats in the same way as
an inheritance tree consiting of only ordinary tables by acquiring the
sample rows from each foreign table on the far side.

Thanks,

Best regards,
Etsuro Fujita

2014-01-27 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/01/25 11:27), Shigeru Hanada wrote:
> Yeah, the consistency is essential for its ease of use. But I'm not sure
> that inherited stats ignoring foreign tables is actually useful for query
> optimization. What I think about the consistency is a) the ANALYZE command
> with no table names skips ANALYZEing inheritance trees that include at least
> one foreign table as a child, but b) the ANALYZE command with a table name
> indicating an inheritance tree that includes any foreign tables does compute
> the inherited stats in the same way as an inheritance tree consiting of only
> ordinary tables by acquiring the sample rows from each foreign table on the
> far side.

b) sounds little complex to understand or explain.

Is it too big change that making ANALYZE command to handle foreign
tables too even if no table name was specified? IIRC, performance
issue was the reason to exclude foreign tables from auto-analyze
processing. ANALYZEing large database contains local huge data also
takes long time. One idea to avoid unexpected long processing is to
add option to foreign tables to mark it as "not-auto-analyzable".

Thoughts?
--
Shigeru HANADA

(2014/01/27 21:49), Shigeru Hanada wrote:
> 2014-01-27 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> (2014/01/25 11:27), Shigeru Hanada wrote:
>> Yeah, the consistency is essential for its ease of use. But I'm not sure
>> that inherited stats ignoring foreign tables is actually useful for query
>> optimization. What I think about the consistency is a) the ANALYZE command
>> with no table names skips ANALYZEing inheritance trees that include at least
>> one foreign table as a child, but b) the ANALYZE command with a table name
>> indicating an inheritance tree that includes any foreign tables does compute
>> the inherited stats in the same way as an inheritance tree consiting of only
>> ordinary tables by acquiring the sample rows from each foreign table on the
>> far side.
>
> b) sounds little complex to understand or explain.
>
> Is it too big change that making ANALYZE command to handle foreign
> tables too even if no table name was specified? IIRC, performance
> issue was the reason to exclude foreign tables from auto-analyze
> processing. ANALYZEing large database contains local huge data also
> takes long time. One idea to avoid unexpected long processing is to
> add option to foreign tables to mark it as "not-auto-analyzable".

Maybe I didn't express my idea clearly. Sorry for that.

I don't think that we now allow the ANALYZE command to handle foreign
tables when no table name is specified with the command. I think that
we allow the ANALYZE command to handle an inheritance tree that includes
foreign tables when the name of the parent table is specified, without
ignoring such foreign tables in the caluculation. ISTM it would be
possible to do so if we introduce a new parameter, say, vac_mode, which
indicates wether vacuum() is called with a specific table or not.

I'll try to modify the ANALYZE command to do so on top of you patch.

Thanks,

Best regards,
Etsuro Fujita

(2014/01/28 22:01), Etsuro Fujita wrote:
> (2014/01/27 21:49), Shigeru Hanada wrote:
>> Is it too big change that making ANALYZE command to handle foreign
>> tables too even if no table name was specified? IIRC, performance
>> issue was the reason to exclude foreign tables from auto-analyze
>> processing. ANALYZEing large database contains local huge data also
>> takes long time. One idea to avoid unexpected long processing is to
>> add option to foreign tables to mark it as "not-auto-analyzable".
>
> Maybe I didn't express my idea clearly. Sorry for that.
>
> I don't think that we now allow the ANALYZE command to handle foreign
> tables when no table name is specified with the command. I think that
> we allow the ANALYZE command to handle an inheritance tree that includes
> foreign tables when the name of the parent table is specified, without
> ignoring such foreign tables in the caluculation. ISTM it would be
> possible to do so if we introduce a new parameter, say, vac_mode, which
> indicates wether vacuum() is called with a specific table or not.
>
> I'll try to modify the ANALYZE command to do so on top of you patch.

Done on top of your patch, foreign_inherit.patch, not the latest
version, foreign_inherit-v2.patch. As I proposed, an inheritance tree
that includes foreign tables is now ANALYZEd, without ignoring such
foreign tables in the inherited-stats computation, if the name of the
parent table is specified with the ANALYZE command. (That has been done
by a small modification of analyze.c, thanks to [1].) The ANALYZE
command with no tablename or the autovacuum worker skips the
inherited-stats computation itself for inheritance trees that includes
foreign tables, which is different from the original patch. To
distinguish the ANALYZE-with-a-tablename command from the others (ie,
the ANALYZE-with-no-tablename command or the autovacuum worker), I've
introduced a new parameter, vacmode, in vacuum(), and thus called
analyze_rel() with that parameter. Attached is the modified patch.
Could you review the patch?

Thanks,

[1]
http://www.postgresql.org/message-id/E1SGFOO-0006ZF-8n@gemulon.postgresql.org

Best regards,
Etsuro Fujita