Re: inherit support for foreign tables

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

Thanks,

Best regards,
Etsuro Fujita