Re: inherit support for foreign tables

Hi Fujita-san,
Here are my review comments for patch fdw-inh-3.patch.

Sanity
--------
1. The patch applies cleanly
2. It compiles cleanly.
3. The server regression tests and tests in contrib modules pass.

Tests
-------
1. It seems like you have copied from testcase inherit.sql to postgres_fdw
testcase. That's a good thing, but it makes the test quite long. May be we
should have two tests in postgres_fdw contrib module, one for simple cases,
and other for inheritance. What do you say?

Documentation
--------------------
1. The change in ddl.sgml
- We will refer to the child tables as partitions, though they
- are in every way normal <productname>PostgreSQL</> tables.
+ We will refer to the child tables as partitions, though we assume
+ that they are normal <productname>PostgreSQL</> tables.

adds phrase "we assume that", which confuses the intention behind the
sentence. The original text is intended to highlight the equivalence
between "partition" and "normal table", where as the addition esp. the word
"assume" weakens that equivalence. Instead now we have to highlight the
equivalence between "partition" and "normal or foreign table". The wording
similar to "though they are either normal or foreign tables" should be used
there.

2. The wording "some kind of optimization" gives vague picture. May be it
can be worded as "Since the constraints are assumed to be true, they are
used in constraint-based query optimization like constraint exclusion for
partitioned tables.".
+ Those constraints are used in some kind of query optimization such
+ as constraint exclusion for partitioned tables (see
+ <xref linkend="ddl-partitioning">).

Code
-------
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from inheritance
tree
*
* This has the same API as acquire_sample_rows, except that rows are
* collected from all inheritance children as well as the specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or there
are
+ * inheritance children that foreign tables.

The addition should be "there are inheritance children that *are all *foreign
tables. Note the addition "are all".

2. The function has_foreign() be better named has_foreign_child()? This
function loops through all the tableoids passed even after it has found a
foreign table. Why can't we return true immediately after finding the first
foreign table, unless the side effects of heap_open() on all the table are
required. But I don't see that to be the case, since these tables are
locked already through a previous call to heap_open(). In the same function
instead of argument name parentrelId may be we should use name parent_oid,
so that we use same notation for parent and child table OIDs.

3. Regarding enum VacuumMode - it's being used only in case of
acquire_inherited_sample_rows() and that too, to check only a single value
of the three defined there. May be we should just infer that value inside
acquire_inherited_sample_rows() or pass a boolean true or false from
do_analyse_rel() based on the VacuumStmt. I do not see need for a separate
three value enum of which only one value is used and also to pass it down
from vacuum() by changing signatures of the minion functions.

4. In postgresGetForeignPlan(), the code added by this patch is required to
handle the case when the row mark is placed on a parent table and hence is
required to be applied on the child table. We need a comment explaining
this. Otherwise, the three step process to get the row mark information
isn't clear for a reader.

5. In expand_inherited_rtentry() why do you need a separate variable
hasForeign? Instead of using that variable, you can actually set/reset
rte->hasForeign since existence of even a single foreign child would mark
that member as true. - After typing this, I got the answer when I looked at
the function code. Every child's RTE is initially a copy of parent's RTE
and hence hasForeign status would be inherited by every child after the
first foreign child. I think, this reasoning should be added as comment
before assignment to rte->hasForeign at the end of the function.

6. The tests in foreign_data.sql are pretty extensive. Thanks for that. I
think, we should also check the effect of each of the following command
using \d on appropriate tables.
+CREATE FOREIGN TABLE ft2 () INHERITS (pt1)
+ SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 NO INHERIT pt1;
+DROP FOREIGN TABLE ft2;
+CREATE FOREIGN TABLE ft2 (
+ c1 integer NOT NULL,
+ c2 text,
+ c3 date
+) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 INHERIT pt1;

Rest of the changes look good.

On Thu, Nov 13, 2014 at 12:21 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

>
>
> On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita <
> fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>> Hi Ashutosh,
>>
>> Thanks for the review!
>>
>> (2014/11/13 15:23), Ashutosh Bapat wrote:
>>
>>> I tried to apply fdw-inh-3.patch on the latest head from master branch.
>>> It failed to apply using both patch and git apply.
>>>
>>> "patch" failed to apply because of rejections in
>>> contrib/file_fdw/output/file_fdw.source and
>>> doc/src/sgml/ref/create_foreign_table.sgml
>>>
>>
>> As I said upthread, fdw-inh-3.patch has been created on top of [1] and
>> fdw-chk-3.patch. Did you apply these patche first?
>>
>> Oh, sorry, I didn't pay attention to that. I will apply both the patches
> and review the inheritance patch. Thanks for pointing that out.
>
>
>> [1] https://commitfest.postgresql.org/action/patch_view?id=1599
>>
>> Best regards,
>> Etsuro Fujita
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>

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