On Mon, Nov 17, 2014 at 1:25 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.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
