Re: inherit support for foreign tables

Hi Fujita-san,
I reviewed fdw-chk-3 patch. Here are my comments

Sanity
--------
1. The patch applies on the latest master using "patch but not by git apply
2. it compiles clean
3. Regression run is clean, including the contrib module regressions

Tests
-------
1. The tests added in file_fdw module look good. We should add tests for
CREATE TABLE with CHECK CONSTRAINT also.
2. For postgres_fdw we need tests to check the behaviour in case the
constraints mismatch between the remote table and its local foreign table
declaration in case of INSERT, UPDATE and SELECT.
3. In the testcases for postgres_fdw it seems that you have forgotten to
add statement after SET constraint_exclusion to 'partition'
4. In test foreign_data there are changes to fix the diffs caused by these
changes like below
ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const; -- ERROR
-ERROR: "ft1" is not a table
+ERROR: constraint "no_const" of relation "ft1" does not exist
ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const;
-ERROR: "ft1" is not a table
+NOTICE: constraint "no_const" of relation "ft1" does not exist, skipping
ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
-ERROR: "ft1" is not a table
+ERROR: constraint "ft1_c1_check" of relation "ft1" does not exist

Earlier when constraints were not supported for FOREIGN TABLE, these tests
made sure the same functionality. So, even though the corresponding
constraints were not created on the table (in fact it didn't allow the
creation as well). Now that the constraints are allowed, I think the tests
for "no_const" (without IF EXISTS) and "ft1_c1_check" are duplicating the
same testcase. May be we should review this set of statement in the light
of new functionality.

Code and implementation
----------------------------------
The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table is
blocked, but corresponding documentation entry doesn't mention so. Since
foreign tables can not be inherited NO INHERIT option isn't applicable to
foreign tables and the constraints on the foreign tables are declarative,
hence NOT VALID option is also not applicable. So, I agree with what the
code is doing, but that should be reflected in documentation with this
explanation.
Rest of the code modifies the condition checks for CHECK CONSTRAINTs on
foreign tables, and it looks good to me.

On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/07 14:57), Kyotaro HORIGUCHI wrote:
>
>> Here are separated patches.
>>>>>
>>>>> fdw-chk.patch - CHECK constraints on foreign tables
>>>>> fdw-inh.patch - table inheritance with foreign tables
>>>>>
>>>>> The latter has been created on top of [1].
>>>>>
>>>>
>>>> [1]
>>>>> http://www.postgresql.org/message-id/540DA168.3040407@lab.ntt.co.jp
>>>>>
>>>>
>>> To be exact, it has been created on top of [1] and fdw-chk.patch.
>>>>
>>>
>> I tried both patches on the current head, the newly added
>> parameter to analyze_rel() hampered them from applying but it is
>> easy to fix seemingly and almost all the other part was applied
>> cleanly.
>>
>
> Thanks for the review!
>
> By the way, are these the result of simply splitting of your last
>> patch, foreign_inherit-v15.patch?
>>
>> http://www.postgresql.org/message-id/53FEEF94.6040101@lab.ntt.co.jp
>>
>
> The answer is "no".
>
> The result of apllying whole-in-one version and this splitted
>> version seem to have many differences. Did you added even other
>> changes? Or do I understand this patch wrongly?
>>
>
> As I said before, I splitted the whole-in-one version into three: 1) CHECK
> constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie
> fdw-inh.patch) and 3) path reparameterization patch (not posted). In
> addition to that, I slightly modified #1 and #2.
>
> IIUC, #3 would be useful not only for the inheritance cases but for union
> all cases. So, I plan to propose it independently in the next CF.
>
> I noticed that the latter disallows TRUNCATE on inheritance trees that
>>>> contain at least one child foreign table. But I think it would be
>>>> better to allow it, with the semantics that we quietly ignore the
>>>> child
>>>> foreign tables and apply the operation to the child plain tables,
>>>> which
>>>> is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
>>>> trees. Comments welcome.
>>>>
>>>
>>> Done. And I've also a bit revised regression tests for both
>>> patches. Patches attached.
>>>
>>
> I rebased the patches to the latest head. Here are updated patches.
>
> Other changes:
>
> * fdw-chk-3.patch: the updated patch revises some ereport messages a
> little bit.
>
> * fdw-inh-3.patch: I noticed that there is a doc bug in the previous
> patch. The updated patch fixes that, adds a bit more docs, and revises
> regression tests in foreign_data.sql a bit further.
>
>
> 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
>
>

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