Hi Fujita-san, I reviewed fdw-chk-3 patch. Here are my comments Sanity -------- 1. The patch applies on the latest master using "patch but not by git apply 2. it compiles clean 3. Regression run is clean, including the contrib module regressions
Tests ------- 1. The tests added in file_fdw module look good. We should add tests for CREATE TABLE with CHECK CONSTRAINT also. 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. 3. In the testcases for postgres_fdw it seems that you have forgotten to add statement after SET constraint_exclusion to 'partition' 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. 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. On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > (2014/11/07 14:57), Kyotaro HORIGUCHI wrote: > >> Here are separated patches. >>>>> >>>>> fdw-chk.patch - CHECK constraints on foreign tables >>>>> fdw-inh.patch - table inheritance with foreign tables >>>>> >>>>> The latter has been created on top of [1]. >>>>> >>>> >>>> [1] >>>>> http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp >>>>> >>>> >>> To be exact, it has been created on top of [1] and fdw-chk.patch. >>>> >>> >> I tried both patches on the current head, the newly added >> parameter to analyze_rel() hampered them from applying but it is >> easy to fix seemingly and almost all the other part was applied >> cleanly. >> > > Thanks for the review! > > By the way, are these the result of simply splitting of your last >> patch, foreign_inherit-v15.patch? >> >> http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp >> > > The answer is "no". > > The result of apllying whole-in-one version and this splitted >> version seem to have many differences. Did you added even other >> changes? Or do I understand this patch wrongly? >> > > As I said before, I splitted the whole-in-one version into three: 1) CHECK > constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie > fdw-inh.patch) and 3) path reparameterization patch (not posted). In > addition to that, I slightly modified #1 and #2. > > IIUC, #3 would be useful not only for the inheritance cases but for union > all cases. So, I plan to propose it independently in the next CF. > > I noticed that the latter disallows TRUNCATE on inheritance trees that >>>> contain at least one child foreign table. But I think it would be >>>> better to allow it, with the semantics that we quietly ignore the >>>> child >>>> foreign tables and apply the operation to the child plain tables, >>>> which >>>> is the same semantics as ALTER COLUMN SET STORAGE on such inheritance >>>> trees. Comments welcome. >>>> >>> >>> Done. And I've also a bit revised regression tests for both >>> patches. Patches attached. >>> >> > I rebased the patches to the latest head. Here are updated patches. > > Other changes: > > * fdw-chk-3.patch: the updated patch revises some ereport messages a > little bit. > > * fdw-inh-3.patch: I noticed that there is a doc bug in the previous > patch. The updated patch fixes that, adds a bit more docs, and revises > regression tests in foreign_data.sql a bit further. > > > Thanks, > > Best regards, > Etsuro Fujita > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company