Postgres Pro
Downloads
Home   >   mailing lists

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

From Ashutosh Bapat
Subject Re: inherit support for foreign tables
Date
Msg-id CAFjFpRdnLEbXCdDf9VVS2dckAoW9UVxiZxo5Rr+a1gcLoD9ABQ@mail.gmail.com
Whole thread Raw
In response to Re: inherit support for foreign tables  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
Responses Re: inherit support for foreign tables  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
List pgsql-hackers
Tree view
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.etsuro@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.3040407@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.6040101@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

pgsql-hackers by date:

Previous
From: Antonin Houska
Date:
Subject: Re: Unintended restart after recovery error
Next
From: Pankaj Bagul
Date:
Subject: how to determine clause for new keyword, how to add new clause in gram.y