(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
