Re: inherit support for foreign tables - Mailing list pgsql-hackers

(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