Re: inherit support for foreign tables

On Mon, Nov 17, 2014 at 1:25 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/12 20:04), Ashutosh Bapat wrote:
>
>> I reviewed fdw-chk-3 patch. Here are my comments
>>
>
> Thanks for the review!
>
> Tests
>> -------
>> 1. The tests added in file_fdw module look good. We should add tests for
>> CREATE TABLE with CHECK CONSTRAINT also.
>>
>
> Agreed. I added the tests, and also changed the proposed tests a bit.
>
> 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.
>>
>
> Done.
>
> 3. In the testcases for postgres_fdw it seems that you have forgotten to
>> add statement after SET constraint_exclusion to 'partition'
>>
>
> I added the statement.
>
> 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.
>>
>
> Agreed. I removed the "DROP CONSTRAINT ft1_c1_check" test, and added a
> new test for ALTER CONSTRAINT.
>
> 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.
>>
>
> Done.
>
> Other changes:
> * Modified one error message that I added in AddRelationNewConstraints, to
> match the other message there.
> * Revised other docs a little bit.
>
> Attached is an updated version of the patch.
>
>
I looked at the patch. It looks good now. Once we have the inh patch ready,
we can mark this item as ready for commiter.

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

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