Re: [HACKERS] foreign partition DDL regression tests
On Thu, Mar 9, 2017 at 11:44 PM, Robert Haaswrote: > On Thu, Mar 9, 2017 at 1:19 AM, Ashutosh Bapat > wrote: At least we need to update the documentation. >>> >>> Got a proposal? >> >> How about something like attached? > > Committed with some revisions. Thanks. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On Thu, Mar 9, 2017 at 1:19 AM, Ashutosh Bapatwrote: >>> At least we need to update the documentation. >> >> Got a proposal? > > How about something like attached? Committed with some revisions. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
> >> I didn't do anything about Ashutosh's comment that we could use ALTER >> FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem >> critical. > > Attached is a patch to fix that, just in case. Thanks. Looks good to me. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
> >> At least we need to update the documentation. > > Got a proposal? How about something like attached? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company attach_part_constraint_doc.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On 2017/03/09 1:28, Robert Haas wrote: > On Tue, Feb 21, 2017 at 8:40 PM, Amit Langote >wrote: >> Ashutosh Bapat pointed out [0] that regression tests are missing for the >> foreign partition DDL commands. Attached patch takes care of that. > > Committed. Thanks. > I didn't do anything about Ashutosh's comment that we could use ALTER > FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem > critical. Attached is a patch to fix that, just in case. > Also, the names of the objects in this test are kinda generic (pt2 et. > al.) but they match the existing names in the same file (pt1, foo). > If we're going to start differentiating those a little better, we > should probably change them all, and as a separate commit. Agreed. Thanks, Amit >From f0467b30b74d2af72480bb20867164ef030f3c56 Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 9 Mar 2017 09:51:39 +0900 Subject: [PATCH] Use ALTER FOREIGN TABLE with foreign table in tests --- src/test/regress/expected/foreign_data.out | 8 src/test/regress/sql/foreign_data.sql | 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index a0f969f3e5..ea197c5e4f 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -1851,11 +1851,11 @@ Server: s0 FDW Options: (delimiter ',', quote '"', "be quoted" 'value') -- cannot add column to a partition -ALTER TABLE pt2_1 ADD c4 char; +ALTER FOREIGN TABLE pt2_1 ADD c4 char; ERROR: cannot add column to a partition -- ok to have a partition's own constraints though -ALTER TABLE pt2_1 ALTER c3 SET NOT NULL; -ALTER TABLE pt2_1 ADD CONSTRAINT p21chk CHECK (c2 <> ''); +ALTER FOREIGN TABLE pt2_1 ALTER c3 SET NOT NULL; +ALTER FOREIGN TABLE pt2_1 ADD CONSTRAINT p21chk CHECK (c2 <> ''); \d+ pt2 Table "public.pt2" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description @@ -1880,7 +1880,7 @@ Server: s0 FDW Options: (delimiter ',', quote '"', "be quoted" 'value') -- cannot drop inherited NOT NULL constraint from a partition -ALTER TABLE pt2_1 ALTER c1 DROP NOT NULL; +ALTER FOREIGN TABLE pt2_1 ALTER c1 DROP NOT NULL; ERROR: column "c1" is marked NOT NULL in parent table -- partition must have parent's constraints ALTER TABLE pt2 DETACH PARTITION pt2_1; diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql index c13d5ffbe9..8c5fcb8b35 100644 --- a/src/test/regress/sql/foreign_data.sql +++ b/src/test/regress/sql/foreign_data.sql @@ -720,16 +720,16 @@ ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1); \d+ pt2_1 -- cannot add column to a partition -ALTER TABLE pt2_1 ADD c4 char; +ALTER FOREIGN TABLE pt2_1 ADD c4 char; -- ok to have a partition's own constraints though -ALTER TABLE pt2_1 ALTER c3 SET NOT NULL; -ALTER TABLE pt2_1 ADD CONSTRAINT p21chk CHECK (c2 <> ''); +ALTER FOREIGN TABLE pt2_1 ALTER c3 SET NOT NULL; +ALTER FOREIGN TABLE pt2_1 ADD CONSTRAINT p21chk CHECK (c2 <> ''); \d+ pt2 \d+ pt2_1 -- cannot drop inherited NOT NULL constraint from a partition -ALTER TABLE pt2_1 ALTER c1 DROP NOT NULL; +ALTER FOREIGN TABLE pt2_1 ALTER c1 DROP NOT NULL; -- partition must have parent's constraints ALTER TABLE pt2 DETACH PARTITION pt2_1; -- 2.11.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On Tue, Feb 21, 2017 at 8:40 PM, Amit Langotewrote: > Ashutosh Bapat pointed out [0] that regression tests are missing for the > foreign partition DDL commands. Attached patch takes care of that. Committed. I didn't do anything about Ashutosh's comment that we could use ALTER FOREIGN TABLE rather than ALTER TABLE someplace; that didn't seem critical. Also, the names of the objects in this test are kinda generic (pt2 et. al.) but they match the existing names in the same file (pt1, foo). If we're going to start differentiating those a little better, we should probably change them all, and as a separate commit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On Tue, Mar 7, 2017 at 11:18 PM, Ashutosh Bapatwrote: >> I agree that we could do that, but what value would it have? It just >> forces the user to spend two SQL commands doing what could otherwise >> be done in one. > > I don't think it's going to be two commands always. A user who wants > to attach a foreign table as a partition, "knows" that the data on the > foreign server honours the partitioning bounds. If s/he knows that > probably he added the constraint on the foreign table, so that planner > could make use of it. Remember this is an existing foreign table. If > s/he is not aware that the data on the foreign server doesn't honour > partition bounds, adding that as a partition would be a problem. I > think, this step gives the user a chance to make a conscious decision. I think attaching the foreign table as a partition constitutes a sufficiently-conscious decision. > At least we need to update the documentation. Got a proposal? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On Wed, Mar 8, 2017 at 7:36 AM, Robert Haaswrote: > On Tue, Mar 7, 2017 at 7:14 AM, Ashutosh Bapat > wrote: >> Hi Amit, >> Thanks for adding testcases. Overall the testcases look good. >> >> The testcase is using ALTER TABLE to modify foreign table schema. >> Though this works, I think a better option is to use ALTER FOREIGN >> TABLE. >> >> Something not related to this patch but >> -- no attach partition validation occurs for foreign tables >> ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1); >> >> I am wondering whether we can improve this. For local tables, if a >> valid constraint equivalent to the partitioning condition is not >> present on the table being attached, it scans the data to verify >> partition conditions. But for a foreign table, we don't want to do >> that since the constraint is not guaranteed to be valid after the >> initial check. For a normal foreign table a user can set a constraint >> if s/he knows that that constraint will be honoured on the foreign >> server. Thus instead of saying that we do not validate data, we can >> refuse to attach a partition if corresponding check constraint is >> absent on the foreign table being attached. A user will then be forced >> to add that constraint if s/he is sure that the constraint will be >> obeyed on the foreign server. > > I agree that we could do that, but what value would it have? It just > forces the user to spend two SQL commands doing what could otherwise > be done in one. I don't think it's going to be two commands always. A user who wants to attach a foreign table as a partition, "knows" that the data on the foreign server honours the partitioning bounds. If s/he knows that probably he added the constraint on the foreign table, so that planner could make use of it. Remember this is an existing foreign table. If s/he is not aware that the data on the foreign server doesn't honour partition bounds, adding that as a partition would be a problem. I think, this step gives the user a chance to make a conscious decision. > And the first command might not be that obvious. A hint with error reported would help. In fact, the hint might as well say "add constraint if you are sure that the foreign data honours partition bound specification" or something like that. I noticed that the documentation at https://www.postgresql.org/docs/devel/static/sql-altertable.html for ATTACH PARTITION does not have anything about foreign tables. May be we should add whatever is the current status. > I > think we should leave well enough alone. At least we need to update the documentation. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On Tue, Mar 7, 2017 at 7:14 AM, Ashutosh Bapatwrote: > Hi Amit, > Thanks for adding testcases. Overall the testcases look good. > > The testcase is using ALTER TABLE to modify foreign table schema. > Though this works, I think a better option is to use ALTER FOREIGN > TABLE. > > Something not related to this patch but > -- no attach partition validation occurs for foreign tables > ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1); > > I am wondering whether we can improve this. For local tables, if a > valid constraint equivalent to the partitioning condition is not > present on the table being attached, it scans the data to verify > partition conditions. But for a foreign table, we don't want to do > that since the constraint is not guaranteed to be valid after the > initial check. For a normal foreign table a user can set a constraint > if s/he knows that that constraint will be honoured on the foreign > server. Thus instead of saying that we do not validate data, we can > refuse to attach a partition if corresponding check constraint is > absent on the foreign table being attached. A user will then be forced > to add that constraint if s/he is sure that the constraint will be > obeyed on the foreign server. I agree that we could do that, but what value would it have? It just forces the user to spend two SQL commands doing what could otherwise be done in one. And the first command might not be that obvious. I think we should leave well enough alone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
Hi Amit, Thanks for adding testcases. Overall the testcases look good. The testcase is using ALTER TABLE to modify foreign table schema. Though this works, I think a better option is to use ALTER FOREIGN TABLE. Something not related to this patch but -- no attach partition validation occurs for foreign tables ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1); I am wondering whether we can improve this. For local tables, if a valid constraint equivalent to the partitioning condition is not present on the table being attached, it scans the data to verify partition conditions. But for a foreign table, we don't want to do that since the constraint is not guaranteed to be valid after the initial check. For a normal foreign table a user can set a constraint if s/he knows that that constraint will be honoured on the foreign server. Thus instead of saying that we do not validate data, we can refuse to attach a partition if corresponding check constraint is absent on the foreign table being attached. A user will then be forced to add that constraint if s/he is sure that the constraint will be obeyed on the foreign server. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
On 2017/02/22 13:26, Ashutosh Bapat wrote: > Please add this to the upcoming commitfest. Done. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] foreign partition DDL regression tests
Please add this to the upcoming commitfest. On Wed, Feb 22, 2017 at 7:10 AM, Amit Langotewrote: > Ashutosh Bapat pointed out [0] that regression tests are missing for the > foreign partition DDL commands. Attached patch takes care of that. > > Thanks, > Amit > > [0] > https://www.postgresql.org/message-id/CAFjFpRcrdzBRj0cZ%2BJAQmfSa2Tv8wSEcWAeYtDpV-YZnNna2sA%40mail.gmail.com > > > -- > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] foreign partition DDL regression tests
Ashutosh Bapat pointed out [0] that regression tests are missing for the foreign partition DDL commands. Attached patch takes care of that. Thanks, Amit [0] https://www.postgresql.org/message-id/CAFjFpRcrdzBRj0cZ%2BJAQmfSa2Tv8wSEcWAeYtDpV-YZnNna2sA%40mail.gmail.com >From 236c357b94af848663ed3d0ace10dd22167b7d08 Mon Sep 17 00:00:00 2001 From: amitDate: Tue, 21 Feb 2017 15:06:04 +0900 Subject: [PATCH] Add regression tests foreign partition DDL Commands like CREATE FOREIGN TABLE .. PARTITION OF, ATTACH PARTITION, DETACH PARTITION foreign_table didn't get any tests so far. Per suggestion from Ashutosh Bapat. --- src/test/regress/expected/foreign_data.out | 195 + src/test/regress/sql/foreign_data.sql | 71 +++ 2 files changed, 266 insertions(+) diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 3a9fb8f558..a0f969f3e5 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -1751,6 +1751,201 @@ DETAIL: user mapping for regress_test_role on server s5 depends on server s5 HINT: Use DROP ... CASCADE to drop the dependent objects too. DROP OWNED BY regress_test_role2 CASCADE; NOTICE: drop cascades to user mapping for regress_test_role on server s5 +-- Foreign partition DDL stuff +CREATE TABLE pt2 ( + c1 integer NOT NULL, + c2 text, + c3 date +) PARTITION BY LIST (c1); +CREATE FOREIGN TABLE pt2_1 PARTITION OF pt2 FOR VALUES IN (1) + SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value'); +\d+ pt2 +Table "public.pt2" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description ++-+---+--+-+--+--+- + c1 | integer | | not null | | plain| | + c2 | text| | | | extended | | + c3 | date| | | | plain| | +Partition key: LIST (c1) +Partitions: pt2_1 FOR VALUES IN (1) + +\d+ pt2_1 + Foreign table "public.pt2_1" + Column | Type | Collation | Nullable | Default | FDW Options | Storage | Stats target | Description ++-+---+--+-+-+--+--+- + c1 | integer | | not null | | | plain| | + c2 | text| | | | | extended | | + c3 | date| | | | | plain| | +Partition of: pt2 FOR VALUES IN (1) +Server: s0 +FDW Options: (delimiter ',', quote '"', "be quoted" 'value') + +-- partition cannot have additional columns +DROP FOREIGN TABLE pt2_1; +CREATE FOREIGN TABLE pt2_1 ( + c1 integer NOT NULL, + c2 text, + c3 date, + c4 char +) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value'); +\d+ pt2_1 + Foreign table "public.pt2_1" + Column | Type | Collation | Nullable | Default | FDW Options | Storage | Stats target | Description ++--+---+--+-+-+--+--+- + c1 | integer | | not null | | | plain| | + c2 | text | | | | | extended | | + c3 | date | | | | | plain| | + c4 | character(1) | | | | | extended | | +Server: s0 +FDW Options: (delimiter ',', quote '"', "be quoted" 'value') + +ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1); -- ERROR +ERROR: table "pt2_1" contains column "c4" not found in parent "pt2" +DETAIL: New partition should contain only the columns present in parent. +DROP FOREIGN TABLE pt2_1; +\d+ pt2 +Table "public.pt2" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description ++-+---+--+-+--+--+- + c1 | integer | | not null | | plain| | + c2 | text| | | | extended | | + c3 | date| | | | plain| | +Partition key: LIST (c1) + +CREATE FOREIGN TABLE pt2_1 ( + c1 integer NOT NULL, + c2 text, + c3 date +) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value'); +\d+ pt2_1 + Foreign table "public.pt2_1" + Column | Type | Collation | Nullable | Default | FDW Options | Storage | Stats target | Description