Just to show what I'm talking about, here are a few prototype patches. Beware, these are all very lightly tested/reviewed. Input is welcome, particularly if it comes in the guise of patches to regression tests showing cases that misbehave.
0001 is a fixup for the v6 patch I posted upthread; it's needed so the 0002 patch doesn't end up with indexes marked invalid after pg_dump. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From bd26504debb14567de780da10bf859b205326b46 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Wed, 20 Dec 2017 16:31:05 -0300 Subject: [PATCH v7 1/4] Fixup! Local partitioned indexes v6 Don't actually mark an index invalid if it's on a plain table, rather than a partition, even with ONLY specified. --- src/backend/commands/indexcmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a757f05dd5..e925351056 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -723,7 +723,7 @@ DefineIndex(Oid relationId, flags |= INDEX_CREATE_PARTITIONED; if (stmt->primary) flags |= INDEX_CREATE_IS_PRIMARY; - if (stmt->relation && !stmt->relation->inh) + if (partitioned && stmt->relation && !stmt->relation->inh) flags |= INDEX_CREATE_INVALID; if (stmt->deferrable) -- 2.11.0
>From b10f9afbe4b1c46c7a2706c70a398307732ac9fc Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Mon, 6 Nov 2017 17:04:55 +0100 Subject: [PATCH v7 2/4] allow indexes on partitioned tables to be unique --- src/backend/commands/indexcmds.c | 71 +++++++++++++-- src/backend/parser/parse_utilcmd.c | 24 ----- src/test/regress/expected/alter_table.out | 8 -- src/test/regress/expected/create_table.out | 12 --- src/test/regress/expected/indexing.out | 142 ++++++++++++++++++++++++++++- src/test/regress/sql/alter_table.sql | 2 - src/test/regress/sql/create_table.sql | 8 -- src/test/regress/sql/indexing.sql | 73 ++++++++++++++- 8 files changed, 274 insertions(+), 66 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index e925351056..86dcab6136 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -426,20 +426,11 @@ DefineIndex(Oid relationId, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot create index on partitioned table \"%s\" concurrently", RelationGetRelationName(rel)))); - if (stmt->unique) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot create unique index on partitioned table \"%s\"", - RelationGetRelationName(rel)))); if (stmt->excludeOpNames) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot create exclusion constraints on partitioned table \"%s\"", RelationGetRelationName(rel)))); - if (stmt->primary || stmt->isconstraint) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot create constraints on partitioned tables"))); } /* @@ -637,6 +628,68 @@ DefineIndex(Oid relationId, index_check_primary_key(rel, indexInfo, is_alter_table); /* + * If this table is partitioned and we're creating a unique index or a + * primary key, make sure that the indexed columns are part of the + * partition key. Otherwise it would be possible to violate uniqueness by + * putting values that ought to be unique in different partitions. + * + * We could lift this limitation if we had global indexes, but those have + * their own problems, so this is a useful feature combination. + */ + if (partitioned && (stmt->unique || stmt->primary)) + { + PartitionKey key = rel->rd_partkey; + int i; + + /* + * A partitioned table can have unique indexes, as long as all the + * columns in the partition key appear in the unique key. A + * partition-local index can enforce global uniqueness iff the PK + * value completely determines the partition that a row is in. + * + * Thus, verify that all the columns in the partition key appear + * in the unique key definition. + */ + for (i = 0; i < key->partnatts; i++) + { + bool found = false; + int j; + + /* + * It may be possible to support UNIQUE constraints when partition + * keys are expressions, but is it worth it? Give up for now. + */ + if (key->partattrs[i] == 0) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + /* XXX reformulate error message? */ + errmsg("UNIQUE constraints are not supported on partitioned tables using expressions as partition keys"))); + + for (j = 0; j < indexInfo->ii_NumIndexAttrs; j++) + { + if (key->partattrs[i] == indexInfo->ii_KeyAttrNumbers[j]) + { + found = true; + break; + } + } + if (!found) + { + Form_pg_attribute att; + + att = TupleDescAttr(RelationGetDescr(rel), key->partattrs[i] - 1); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("insufficient columns in UNIQUE constraint definition"), + errdetail("UNIQUE constraint on table \"%s\" does not include column \"%s\" which is part of the partition key.", + RelationGetRelationName(rel), + NameStr(att->attname)))); + } + } + } + + + /* * We disallow indexes on system columns other than OID. They would not * necessarily get updated correctly, and they don't seem useful anyway. */ diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 45f6ec2820..e80edd3668 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -704,12 +704,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column) errmsg("primary key constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("primary key constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); /* FALL THRU */ case CONSTR_UNIQUE: @@ -719,12 +713,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column) errmsg("unique constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("unique constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); if (constraint->keys == NIL) constraint->keys = list_make1(makeString(column->colname)); cxt->ixconstraints = lappend(cxt->ixconstraints, constraint); @@ -821,12 +809,6 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint) errmsg("primary key constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("primary key constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); cxt->ixconstraints = lappend(cxt->ixconstraints, constraint); break; @@ -837,12 +819,6 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint) errmsg("unique constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("unique constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); cxt->ixconstraints = lappend(cxt->ixconstraints, constraint); break; diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 517fb080bd..2caf930242 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3290,14 +3290,6 @@ CREATE TABLE partitioned ( a int, b int ) PARTITION BY RANGE (a, (a+b+1)); -ALTER TABLE partitioned ADD UNIQUE (a); -ERROR: unique constraints are not supported on partitioned tables -LINE 1: ALTER TABLE partitioned ADD UNIQUE (a); - ^ -ALTER TABLE partitioned ADD PRIMARY KEY (a); -ERROR: primary key constraints are not supported on partitioned tables -LINE 1: ALTER TABLE partitioned ADD PRIMARY KEY (a); - ^ ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; ERROR: foreign key constraints are not supported on partitioned tables LINE 1: ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 8e745402ae..866cc99b9f 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -276,12 +276,6 @@ CREATE TABLE partitioned ( ) PARTITION BY LIST (a1, a2); -- fail ERROR: cannot use "list" partition strategy with more than one column -- unsupported constraint type for partitioned tables -CREATE TABLE partitioned ( - a int PRIMARY KEY -) PARTITION BY RANGE (a); -ERROR: primary key constraints are not supported on partitioned tables -LINE 2: a int PRIMARY KEY - ^ CREATE TABLE pkrel ( a int PRIMARY KEY ); @@ -293,12 +287,6 @@ LINE 2: a int REFERENCES pkrel(a) ^ DROP TABLE pkrel; CREATE TABLE partitioned ( - a int UNIQUE -) PARTITION BY RANGE (a); -ERROR: unique constraints are not supported on partitioned tables -LINE 2: a int UNIQUE - ^ -CREATE TABLE partitioned ( a int, EXCLUDE USING gist (a WITH &&) ) PARTITION BY RANGE (a); diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index 2904ad4cf4..812e570abc 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -25,8 +25,6 @@ drop table idxpart; -- Some unsupported features create table idxpart (a int, b int, c text) partition by range (a); create table idxpart1 partition of idxpart for values from (0) to (10); -create unique index on idxpart (a); -ERROR: cannot create unique index on partitioned table "idxpart" create index concurrently on idxpart (a); ERROR: cannot create index on partitioned table "idxpart" concurrently drop table idxpart; @@ -404,3 +402,143 @@ select attrelid::regclass, attname, attnum from pg_attribute (7 rows) drop table idxpart; +-- +-- Constraint-related indexes +-- +-- Verify that it works to add primary key / unique to partitioned tables +create table idxpart (a int primary key, b int) partition by range (a); +\d idxpart + Table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | not null | + b | integer | | | +Partition key: RANGE (a) +Indexes: + "idxpart_pkey" PRIMARY KEY, btree (a) +Number of partitions: 0 + +drop table idxpart; +-- but not if you fail to use the full partition key +create table idxpart (a int unique, b int) partition by range (a, b); +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "b" which is part of the partition key. +create table idxpart (a int, b int unique) partition by range (a, b); +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "a" which is part of the partition key. +create table idxpart (a int primary key, b int) partition by range (b, a); +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "b" which is part of the partition key. +create table idxpart (a int, b int primary key) partition by range (b, a); +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "a" which is part of the partition key. +-- OK if you use them in some other order +create table idxpart (a int, b int, c text, primary key (a, b, c)) partition by range (b, c, a); +drop table idxpart; +create table idxpart (a int primary key, b int) partition by range ((b + a)); +ERROR: UNIQUE constraints are not supported on partitioned tables using expressions as partition keys +-- not other types of index-based constraints +create table idxpart (a int, exclude (a with = )) partition by range (a); +ERROR: exclusion constraints are not supported on partitioned tables +LINE 1: create table idxpart (a int, exclude (a with = )) partition ... + ^ +-- It works to add primary keys after the partitioned table is created +create table idxpart (a int, b int, c text) partition by range (a, b); +alter table idxpart add primary key (a); -- not an incomplete one tho +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "b" which is part of the partition key. +alter table idxpart add primary key (a, b); +\d idxpart + Table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | not null | + b | integer | | not null | + c | text | | | +Partition key: RANGE (a, b) +Indexes: + "idxpart_pkey" PRIMARY KEY, btree (a, b) +Number of partitions: 0 + +create table idxpart1 partition of idxpart for values from (0, 0) to (1000, 1000); +\d idxpart1 + Table "public.idxpart1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | not null | + b | integer | | not null | + c | text | | | +Partition of: idxpart FOR VALUES FROM (0, 0) TO (1000, 1000) +Indexes: + "idxpart1_pkey" PRIMARY KEY, btree (a, b) + +drop table idxpart; +-- It works to add unique constraints after the partitioned table is created +create table idxpart (a int, b int) partition by range (a, b); +alter table idxpart add unique (a); -- ... nope +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart" does not include column "b" which is part of the partition key. +alter table idxpart add unique (b, a); +\d idxpart + Table "public.idxpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition key: RANGE (a, b) +Indexes: + "idxpart_b_a_key" UNIQUE CONSTRAINT, btree (b, a) +Number of partitions: 0 + +drop table idxpart; +-- Exclusion constraints cannot be added +create table idxpart (a int, b int) partition by range (a); +alter table idxpart add exclude (a with =); +ERROR: exclusion constraints are not supported on partitioned tables +LINE 1: alter table idxpart add exclude (a with =); + ^ +drop table idxpart; +-- When (sub)partitions are created, they also contain the constraint +create table idxpart (a int, b int, primary key (a, b)) partition by range (a, b); +create table idxpart1 partition of idxpart for values from (1, 1) to (10, 10); +create table idxpart2 partition of idxpart for values from (10, 10) to (20, 20) + partition by range (b); +create table idxpart21 partition of idxpart2 for values from (10) to (15); +create table idxpart22 partition of idxpart2 for values from (15) to (20); +create table idxpart3 (b int not null, a int not null); +alter table idxpart attach partition idxpart3 for values from (20, 20) to (30, 30); +select conname, contype, conrelid::regclass, conindid::regclass, conkey + from pg_constraint where conrelid::regclass::text like 'idxpart%' + order by conname; + conname | contype | conrelid | conindid | conkey +----------------+---------+-----------+----------------+-------- + idxpart1_pkey | p | idxpart1 | idxpart1_pkey | {1,2} + idxpart21_pkey | p | idxpart21 | idxpart21_pkey | {1,2} + idxpart22_pkey | p | idxpart22 | idxpart22_pkey | {1,2} + idxpart2_pkey | p | idxpart2 | idxpart2_pkey | {1,2} + idxpart3_pkey | p | idxpart3 | idxpart3_pkey | {2,1} + idxpart_pkey | p | idxpart | idxpart_pkey | {1,2} +(6 rows) + +drop table idxpart; +-- multi-layer partitioning honors the prohibition. So this fails: +create table idxpart (a int, b int, primary key (a)) partition by range (a); +create table idxpart2 partition of idxpart for values from (0) to (1000) partition by range (b); +ERROR: insufficient columns in UNIQUE constraint definition +DETAIL: UNIQUE constraint on table "idxpart2" does not include column "b" which is part of the partition key. +drop table idxpart; +-- but this works: +create table idxpart (a int, b int, primary key (a, b)) partition by range (a); +create table idxpart2 partition of idxpart for values from (0) to (1000) partition by range (b); +create table idxpart21 partition of idxpart2 for values from (0) to (1000); +select conname, contype, conrelid::regclass, conindid::regclass, conkey + from pg_constraint where conrelid::regclass::text like 'idxpart%' + order by conname; + conname | contype | conrelid | conindid | conkey +----------------+---------+-----------+----------------+-------- + idxpart21_pkey | p | idxpart21 | idxpart21_pkey | {1,2} + idxpart2_pkey | p | idxpart2 | idxpart2_pkey | {1,2} + idxpart_pkey | p | idxpart | idxpart_pkey | {1,2} +(3 rows) + +drop table idxpart; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index af25ee9e77..ed0bb7845b 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2016,8 +2016,6 @@ CREATE TABLE partitioned ( a int, b int ) PARTITION BY RANGE (a, (a+b+1)); -ALTER TABLE partitioned ADD UNIQUE (a); -ALTER TABLE partitioned ADD PRIMARY KEY (a); ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&); diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 8f9991ef18..fefccf21a2 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -294,10 +294,6 @@ CREATE TABLE partitioned ( ) PARTITION BY LIST (a1, a2); -- fail -- unsupported constraint type for partitioned tables -CREATE TABLE partitioned ( - a int PRIMARY KEY -) PARTITION BY RANGE (a); - CREATE TABLE pkrel ( a int PRIMARY KEY ); @@ -307,10 +303,6 @@ CREATE TABLE partitioned ( DROP TABLE pkrel; CREATE TABLE partitioned ( - a int UNIQUE -) PARTITION BY RANGE (a); - -CREATE TABLE partitioned ( a int, EXCLUDE USING gist (a WITH &&) ) PARTITION BY RANGE (a); diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index 06b240ff54..bb23cd8777 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -14,7 +14,6 @@ drop table idxpart; -- Some unsupported features create table idxpart (a int, b int, c text) partition by range (a); create table idxpart1 partition of idxpart for values from (0) to (10); -create unique index on idxpart (a); create index concurrently on idxpart (a); drop table idxpart; @@ -171,3 +170,75 @@ select attrelid::regclass, attname, attnum from pg_attribute where attrelid::regclass::text like 'idxpart%' and attnum > 0 order by attrelid::regclass, attnum; drop table idxpart; + +-- +-- Constraint-related indexes +-- + +-- Verify that it works to add primary key / unique to partitioned tables +create table idxpart (a int primary key, b int) partition by range (a); +\d idxpart +drop table idxpart; + +-- but not if you fail to use the full partition key +create table idxpart (a int unique, b int) partition by range (a, b); +create table idxpart (a int, b int unique) partition by range (a, b); +create table idxpart (a int primary key, b int) partition by range (b, a); +create table idxpart (a int, b int primary key) partition by range (b, a); + +-- OK if you use them in some other order +create table idxpart (a int, b int, c text, primary key (a, b, c)) partition by range (b, c, a); +drop table idxpart; + +create table idxpart (a int primary key, b int) partition by range ((b + a)); +-- not other types of index-based constraints +create table idxpart (a int, exclude (a with = )) partition by range (a); + +-- It works to add primary keys after the partitioned table is created +create table idxpart (a int, b int, c text) partition by range (a, b); +alter table idxpart add primary key (a); -- not an incomplete one tho +alter table idxpart add primary key (a, b); +\d idxpart +create table idxpart1 partition of idxpart for values from (0, 0) to (1000, 1000); +\d idxpart1 +drop table idxpart; + +-- It works to add unique constraints after the partitioned table is created +create table idxpart (a int, b int) partition by range (a, b); +alter table idxpart add unique (a); -- ... nope +alter table idxpart add unique (b, a); +\d idxpart +drop table idxpart; + +-- Exclusion constraints cannot be added +create table idxpart (a int, b int) partition by range (a); +alter table idxpart add exclude (a with =); +drop table idxpart; + +-- When (sub)partitions are created, they also contain the constraint +create table idxpart (a int, b int, primary key (a, b)) partition by range (a, b); +create table idxpart1 partition of idxpart for values from (1, 1) to (10, 10); +create table idxpart2 partition of idxpart for values from (10, 10) to (20, 20) + partition by range (b); +create table idxpart21 partition of idxpart2 for values from (10) to (15); +create table idxpart22 partition of idxpart2 for values from (15) to (20); +create table idxpart3 (b int not null, a int not null); +alter table idxpart attach partition idxpart3 for values from (20, 20) to (30, 30); +select conname, contype, conrelid::regclass, conindid::regclass, conkey + from pg_constraint where conrelid::regclass::text like 'idxpart%' + order by conname; +drop table idxpart; + +-- multi-layer partitioning honors the prohibition. So this fails: +create table idxpart (a int, b int, primary key (a)) partition by range (a); +create table idxpart2 partition of idxpart for values from (0) to (1000) partition by range (b); +drop table idxpart; + +-- but this works: +create table idxpart (a int, b int, primary key (a, b)) partition by range (a); +create table idxpart2 partition of idxpart for values from (0) to (1000) partition by range (b); +create table idxpart21 partition of idxpart2 for values from (0) to (1000); +select conname, contype, conrelid::regclass, conindid::regclass, conkey + from pg_constraint where conrelid::regclass::text like 'idxpart%' + order by conname; +drop table idxpart; -- 2.11.0
>From 52e0de2aa3ac6941c429a237ad3123902cda5f10 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 21 Nov 2017 15:53:11 -0300 Subject: [PATCH v7 3/4] Allow FOR EACH ROW triggers on partitioned tables --- src/backend/commands/trigger.c | 73 +++++++++++++++++++++++++++++++--- src/test/regress/expected/triggers.out | 55 ++++++++++++++++++++----- src/test/regress/sql/triggers.sql | 42 ++++++++++++++----- 3 files changed, 142 insertions(+), 28 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 92ae3822d8..15a3cd0bff 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -133,6 +133,10 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType); * relation, as well as ACL_EXECUTE on the trigger function. For internal * triggers the caller must apply any required permission checks. * + * When called on partitioned tables, this function recurses to create the + * trigger on all the partitions, except if isInternal is true, in which + * case caller is expected to execute recursion on its own. + * * Note: can return InvalidObjectAddress if we decided to not create a trigger * at all, but a foreign-key constraint. This is a kluge for backwards * compatibility. @@ -179,8 +183,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, * Triggers must be on tables or views, and there are additional * relation-type-specific restrictions. */ - if (rel->rd_rel->relkind == RELKIND_RELATION || - rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + if (rel->rd_rel->relkind == RELKIND_RELATION) { /* Tables can't have INSTEAD OF triggers */ if (stmt->timing != TRIGGER_TYPE_BEFORE && @@ -190,13 +193,46 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, errmsg("\"%s\" is a table", RelationGetRelationName(rel)), errdetail("Tables cannot have INSTEAD OF triggers."))); - /* Disallow ROW triggers on partitioned tables */ - if (stmt->row && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + } + else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + /* Partitioned tables can't have INSTEAD OF triggers */ + if (stmt->timing != TRIGGER_TYPE_BEFORE && + stmt->timing != TRIGGER_TYPE_AFTER) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is a partitioned table", + errmsg("\"%s\" is a table", RelationGetRelationName(rel)), - errdetail("Partitioned tables cannot have ROW triggers."))); + errdetail("Tables cannot have INSTEAD OF triggers."))); + + /* + * FOR EACH ROW triggers have further restrictions + */ + if (stmt->row) + { + /* + * Disallow WHEN clauses; I think it's okay, but disallow for now + * to reduce testing surface. + */ + if (stmt->whenClause) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is a partitioned table", + RelationGetRelationName(rel)), + errdetail("Triggers FOR EACH ROW on partitioned table cannot have WHEN clauses."))); + + /* + * BEFORE triggers FOR EACH ROW are forbidden, because they would + * allow the user to direct the row to another partition, which + * isn't implemented in the executor. + */ + if (stmt->timing != TRIGGER_TYPE_AFTER) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is a partitioned table", + RelationGetRelationName(rel)), + errdetail("Partitioned tables cannot have BEFORE / FOR EACH ROW triggers."))); + } } else if (rel->rd_rel->relkind == RELKIND_VIEW) { @@ -982,6 +1018,31 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, InvokeObjectPostCreateHookArg(TriggerRelationId, trigoid, 0, isInternal); + /* + * If this is a FOR EACH ROW trigger on a partitioned table, recurse for + * each partition if invoked directly by user (otherwise, caller must do + * its own recursion). + */ + if (stmt->row && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && + !isInternal) + { + PartitionDesc partdesc = RelationGetPartitionDesc(rel); + int i; + + for (i = 0; i < partdesc->nparts; i++) + { + /* XXX must create a separate constraint for each child */ + Assert(constraintOid == InvalidOid); + /* XXX must create a separate index for each child */ + Assert(indexOid == InvalidOid); + + CreateTrigger(copyObject(stmt), queryString, + partdesc->oids[i], refRelOid, + constraintOid, indexOid, + isInternal); + } + } + /* Keep lock on target rel until end of xact */ heap_close(rel, NoLock); diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 85d948741e..e3a88aceca 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -1824,7 +1824,7 @@ create or replace function trigger_notice() returns trigger as $$ return null; end; $$ language plpgsql; --- insert/update/delete statment-level triggers on the parent +-- insert/update/delete statement-level triggers on the parent create trigger trig_ins_before before insert on parted_stmt_trig for each statement execute procedure trigger_notice(); create trigger trig_ins_after after insert on parted_stmt_trig @@ -1837,27 +1837,51 @@ create trigger trig_del_before before delete on parted_stmt_trig for each statement execute procedure trigger_notice(); create trigger trig_del_after after delete on parted_stmt_trig for each statement execute procedure trigger_notice(); +-- these cases are disallowed +create trigger trig_ins_before_1 before insert on parted_stmt_trig + for each row execute procedure trigger_notice(); +ERROR: "parted_stmt_trig" is a partitioned table +DETAIL: Partitioned tables cannot have BEFORE / FOR EACH ROW triggers. +create trigger trig_upd_before_1 before update on parted_stmt_trig + for each row execute procedure trigger_notice(); +ERROR: "parted_stmt_trig" is a partitioned table +DETAIL: Partitioned tables cannot have BEFORE / FOR EACH ROW triggers. +create trigger trig_del_before_1 before delete on parted_stmt_trig + for each row execute procedure trigger_notice(); +ERROR: "parted_stmt_trig" is a partitioned table +DETAIL: Partitioned tables cannot have BEFORE / FOR EACH ROW triggers. +-- insert/update/delete row-level triggers on the parent +create trigger trig_ins_after_parent after insert on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_upd_after_parent after update on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_del_after_parent after delete on parted_stmt_trig + for each row execute procedure trigger_notice(); -- insert/update/delete row-level triggers on the first partition -create trigger trig_ins_before before insert on parted_stmt_trig1 +create trigger trig_ins_before_child before insert on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_ins_after after insert on parted_stmt_trig1 +create trigger trig_ins_after_child after insert on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_upd_before before update on parted_stmt_trig1 +create trigger trig_upd_before_child before update on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_upd_after after update on parted_stmt_trig1 +create trigger trig_upd_after_child after update on parted_stmt_trig1 + for each row execute procedure trigger_notice(); +create trigger trig_del_before_child before delete on parted_stmt_trig1 + for each row execute procedure trigger_notice(); +create trigger trig_del_after_child after delete on parted_stmt_trig1 for each row execute procedure trigger_notice(); -- insert/update/delete statement-level triggers on the parent -create trigger trig_ins_before before insert on parted2_stmt_trig +create trigger trig_ins_before_3 before insert on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_ins_after after insert on parted2_stmt_trig +create trigger trig_ins_after_3 after insert on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_upd_before before update on parted2_stmt_trig +create trigger trig_upd_before_3 before update on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_upd_after after update on parted2_stmt_trig +create trigger trig_upd_after_3 after update on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_del_before before delete on parted2_stmt_trig +create trigger trig_del_before_3 before delete on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_del_after after delete on parted2_stmt_trig +create trigger trig_del_after_3 after delete on parted2_stmt_trig for each statement execute procedure trigger_notice(); with ins (a) as ( insert into parted2_stmt_trig values (1), (2) returning a @@ -1866,6 +1890,8 @@ NOTICE: trigger on parted_stmt_trig BEFORE INSERT for STATEMENT NOTICE: trigger on parted2_stmt_trig BEFORE INSERT for STATEMENT NOTICE: trigger on parted_stmt_trig1 BEFORE INSERT for ROW NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW +NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW +NOTICE: trigger on parted_stmt_trig2 AFTER INSERT for ROW NOTICE: trigger on parted2_stmt_trig AFTER INSERT for STATEMENT NOTICE: trigger on parted_stmt_trig AFTER INSERT for STATEMENT tableoid | a @@ -1881,21 +1907,28 @@ NOTICE: trigger on parted_stmt_trig BEFORE UPDATE for STATEMENT NOTICE: trigger on parted_stmt_trig1 BEFORE UPDATE for ROW NOTICE: trigger on parted2_stmt_trig BEFORE UPDATE for STATEMENT NOTICE: trigger on parted_stmt_trig1 AFTER UPDATE for ROW +NOTICE: trigger on parted_stmt_trig1 AFTER UPDATE for ROW +NOTICE: trigger on parted_stmt_trig2 AFTER UPDATE for ROW NOTICE: trigger on parted_stmt_trig AFTER UPDATE for STATEMENT NOTICE: trigger on parted2_stmt_trig AFTER UPDATE for STATEMENT delete from parted_stmt_trig; NOTICE: trigger on parted_stmt_trig BEFORE DELETE for STATEMENT +NOTICE: trigger on parted_stmt_trig1 BEFORE DELETE for ROW +NOTICE: trigger on parted_stmt_trig2 AFTER DELETE for ROW NOTICE: trigger on parted_stmt_trig AFTER DELETE for STATEMENT -- insert via copy on the parent copy parted_stmt_trig(a) from stdin; NOTICE: trigger on parted_stmt_trig BEFORE INSERT for STATEMENT NOTICE: trigger on parted_stmt_trig1 BEFORE INSERT for ROW NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW +NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW +NOTICE: trigger on parted_stmt_trig2 AFTER INSERT for ROW NOTICE: trigger on parted_stmt_trig AFTER INSERT for STATEMENT -- insert via copy on the first partition copy parted_stmt_trig1(a) from stdin; NOTICE: trigger on parted_stmt_trig1 BEFORE INSERT for ROW NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW +NOTICE: trigger on parted_stmt_trig1 AFTER INSERT for ROW drop table parted_stmt_trig, parted2_stmt_trig; -- -- Test the interaction between transition tables and both kinds of diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 2b2236ed7d..7b1f17cf9a 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1307,7 +1307,7 @@ create or replace function trigger_notice() returns trigger as $$ end; $$ language plpgsql; --- insert/update/delete statment-level triggers on the parent +-- insert/update/delete statement-level triggers on the parent create trigger trig_ins_before before insert on parted_stmt_trig for each statement execute procedure trigger_notice(); create trigger trig_ins_after after insert on parted_stmt_trig @@ -1321,28 +1321,48 @@ create trigger trig_del_before before delete on parted_stmt_trig create trigger trig_del_after after delete on parted_stmt_trig for each statement execute procedure trigger_notice(); +-- these cases are disallowed +create trigger trig_ins_before_1 before insert on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_upd_before_1 before update on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_del_before_1 before delete on parted_stmt_trig + for each row execute procedure trigger_notice(); + +-- insert/update/delete row-level triggers on the parent +create trigger trig_ins_after_parent after insert on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_upd_after_parent after update on parted_stmt_trig + for each row execute procedure trigger_notice(); +create trigger trig_del_after_parent after delete on parted_stmt_trig + for each row execute procedure trigger_notice(); + -- insert/update/delete row-level triggers on the first partition -create trigger trig_ins_before before insert on parted_stmt_trig1 +create trigger trig_ins_before_child before insert on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_ins_after after insert on parted_stmt_trig1 +create trigger trig_ins_after_child after insert on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_upd_before before update on parted_stmt_trig1 +create trigger trig_upd_before_child before update on parted_stmt_trig1 for each row execute procedure trigger_notice(); -create trigger trig_upd_after after update on parted_stmt_trig1 +create trigger trig_upd_after_child after update on parted_stmt_trig1 + for each row execute procedure trigger_notice(); +create trigger trig_del_before_child before delete on parted_stmt_trig1 + for each row execute procedure trigger_notice(); +create trigger trig_del_after_child after delete on parted_stmt_trig1 for each row execute procedure trigger_notice(); -- insert/update/delete statement-level triggers on the parent -create trigger trig_ins_before before insert on parted2_stmt_trig +create trigger trig_ins_before_3 before insert on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_ins_after after insert on parted2_stmt_trig +create trigger trig_ins_after_3 after insert on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_upd_before before update on parted2_stmt_trig +create trigger trig_upd_before_3 before update on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_upd_after after update on parted2_stmt_trig +create trigger trig_upd_after_3 after update on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_del_before before delete on parted2_stmt_trig +create trigger trig_del_before_3 before delete on parted2_stmt_trig for each statement execute procedure trigger_notice(); -create trigger trig_del_after after delete on parted2_stmt_trig +create trigger trig_del_after_3 after delete on parted2_stmt_trig for each statement execute procedure trigger_notice(); with ins (a) as ( -- 2.11.0
>From b52a32a3b67df4b7c1f7824c923ee4b4f3a90278 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 21 Nov 2017 15:54:14 -0300 Subject: [PATCH v7 4/4] [WIP] Allow foreign key triggers on partitioned tables --- src/backend/catalog/pg_constraint.c | 192 +++++++++++++++++++++++++++++ src/backend/commands/tablecmds.c | 105 +++++++++++++--- src/backend/parser/parse_utilcmd.c | 12 -- src/backend/utils/adt/ri_triggers.c | 50 +++----- src/include/catalog/pg_constraint_fn.h | 2 + src/include/commands/tablecmds.h | 4 + src/test/regress/expected/alter_table.out | 4 - src/test/regress/expected/create_table.out | 10 -- src/test/regress/sql/alter_table.sql | 49 +++++++- src/test/regress/sql/create_table.sql | 8 -- src/test/regress/sql/foreign_key.sql | 30 +++++ 11 files changed, 382 insertions(+), 84 deletions(-) diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 7dee6db0eb..9dc91fb67f 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -26,6 +26,7 @@ #include "catalog/pg_operator.h" #include "catalog/pg_type.h" #include "commands/defrem.h" +#include "commands/tablecmds.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/fmgroids.h" @@ -375,6 +376,197 @@ CreateConstraintEntry(const char *constraintName, return conOid; } +/* + * For each foreign key constraint in relation parentId, create a cloned + * copy of it for relationId. + * + * relationId is a partition of parentId, so we can be certain that it has + * the same columns with the same datatypes. They may be in different order, + * though. + */ +void +CloneForeignKeyConstraints(Oid parentId, Oid relationId) +{ + Relation pg_constraint; + Relation rel; + ScanKeyData key; + SysScanDesc scan; + TupleDesc tupdesc; + HeapTuple tuple; + + /* see ATAddForeignKeyConstraint about lock level */ + rel = heap_open(relationId, AccessExclusiveLock); + + pg_constraint = heap_open(ConstraintRelationId, RowShareLock); + tupdesc = RelationGetDescr(pg_constraint); + + ScanKeyInit(&key, + Anum_pg_constraint_conrelid, BTEqualStrategyNumber, + F_OIDEQ, ObjectIdGetDatum(parentId)); + scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true, + NULL, 1, &key); + + while ((tuple = systable_getnext(scan)) != NULL) + { + Form_pg_constraint constrForm = (Form_pg_constraint) GETSTRUCT(tuple); + AttrNumber conkey[INDEX_MAX_KEYS]; + AttrNumber confkey[INDEX_MAX_KEYS]; + Oid conpfeqop[INDEX_MAX_KEYS]; + Oid conppeqop[INDEX_MAX_KEYS]; + Oid conffeqop[INDEX_MAX_KEYS]; + Constraint *fkconstraint; + Oid constrOid; + ObjectAddress parentAddr, + childAddr; + int nelem; + ArrayType *arr; + Datum datum; + bool isnull; + + /* only foreign keys */ + if (constrForm->contype != CONSTRAINT_FOREIGN) + continue; + + ObjectAddressSet(parentAddr, ConstraintRelationId, + HeapTupleGetOid(tuple)); + + datum = fastgetattr(tuple, Anum_pg_constraint_conkey, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null conkey"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != INT2OID) + elog(ERROR, "conkey is not a 1-D smallint array"); + memcpy(conkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber)); + + datum = fastgetattr(tuple, Anum_pg_constraint_confkey, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null confkey"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != INT2OID) + elog(ERROR, "confkey is not a 1-D smallint array"); + memcpy(confkey, ARR_DATA_PTR(arr), nelem * sizeof(AttrNumber)); + + datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null conpfeqop"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != OIDOID) + elog(ERROR, "conpfeqop is not a 1-D OID array"); + memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); + + datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null conpfeqop"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != OIDOID) + elog(ERROR, "conpfeqop is not a 1-D OID array"); + memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); + + datum = fastgetattr(tuple, Anum_pg_constraint_conppeqop, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null conppeqop"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != OIDOID) + elog(ERROR, "conppeqop is not a 1-D OID array"); + memcpy(conppeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); + + datum = fastgetattr(tuple, Anum_pg_constraint_conffeqop, + tupdesc, &isnull); + if (isnull) + elog(ERROR, "null conffeqop"); + arr = DatumGetArrayTypeP(datum); + nelem = ARR_DIMS(arr)[0]; + if (ARR_NDIM(arr) != 1 || + nelem < 1 || + nelem > INDEX_MAX_KEYS || + ARR_HASNULL(arr) || + ARR_ELEMTYPE(arr) != OIDOID) + elog(ERROR, "conffeqop is not a 1-D OID array"); + memcpy(conffeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid)); + + constrOid = + CreateConstraintEntry(NameStr(constrForm->conname), + constrForm->connamespace, + CONSTRAINT_FOREIGN, + constrForm->condeferrable, + constrForm->condeferred, + constrForm->convalidated, + relationId, + conkey, + nelem, + InvalidOid, /* not a domain constraint */ + constrForm->conindid, /* same index */ + constrForm->confrelid, /* same foreign rel */ + confkey, + conpfeqop, + conppeqop, + conffeqop, + nelem, + constrForm->confupdtype, + constrForm->confdeltype, + constrForm->confmatchtype, + NULL, + NULL, + NULL, + NULL, + false, + 1, false, true); + + ObjectAddressSet(childAddr, ConstraintRelationId, constrOid); + recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_INTERNAL); + + fkconstraint = makeNode(Constraint); + /* for now this is all we need */ + fkconstraint->fk_upd_action = constrForm->confupdtype; + fkconstraint->fk_del_action = constrForm->confdeltype; + fkconstraint->deferrable = constrForm->condeferrable; + fkconstraint->initdeferred = constrForm->condeferred; + + createForeignKeyTriggers(rel, constrForm->confrelid, fkconstraint, + constrOid, constrForm->conindid); + + /* + * XXX Normal constraint creation can be invoked during ALTER and + * so it needs ALTER TABLE's phase 3 checking. Current caller is just + * CREATE TABLE .. PARTITION OF so we don't need it, but maybe for + * ALTER TABLE .. ATTACH PARTITION we'll need it. + */ + } + systable_endscan(scan); + + heap_close(rel, NoLock); /* keep lock till commit */ + heap_close(pg_constraint, RowShareLock); +} /* * Test whether given name is currently used as a constraint name diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 47cb7c13ef..e59d774a1b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -337,9 +337,6 @@ static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, Oid pkindOid, Oid constraintOid); -static void createForeignKeyTriggers(Relation rel, Oid refRelOid, - Constraint *fkconstraint, - Oid constraintOid, Oid indexOid); static void ATController(AlterTableStmt *parsetree, Relation rel, List *cmds, bool recurse, LOCKMODE lockmode); static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, @@ -410,8 +407,10 @@ static ObjectAddress ATAddCheckConstraint(List **wqueue, Constraint *constr, bool recurse, bool recursing, bool is_readd, LOCKMODE lockmode); -static ObjectAddress ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, - Constraint *fkconstraint, LOCKMODE lockmode); +static ObjectAddress ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, + Relation rel, + Constraint *fkconstraint, bool recurse, bool recursing, + LOCKMODE lockmode); static void ATExecDropConstraint(Relation rel, const char *constrName, DropBehavior behavior, bool recurse, bool recursing, @@ -503,6 +502,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx, * relkind: relkind to assign to the new relation * ownerId: if not InvalidOid, use this as the new relation's owner. * typaddress: if not null, it's set to the pg_type entry's address. + * queryString: for error reporting * * Note that permissions checks are done against current user regardless of * ownerId. A nonzero ownerId is used when someone is creating a relation @@ -955,6 +955,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, } list_free(idxlist); + + CloneForeignKeyConstraints(parentId, relationId); + heap_close(parent, NoLock); } @@ -7034,7 +7037,8 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, RelationGetNamespace(rel), NIL); - address = ATAddForeignKeyConstraint(tab, rel, newConstraint, + address = ATAddForeignKeyConstraint(wqueue, tab, rel, + newConstraint, recurse, false, lockmode); break; @@ -7189,8 +7193,9 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * We do permissions checks here, however. */ static ObjectAddress -ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, - Constraint *fkconstraint, LOCKMODE lockmode) +ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, + Constraint *fkconstraint, bool recurse, + bool recursing, LOCKMODE lockmode) { Relation pkrel; int16 pkattnum[INDEX_MAX_KEYS]; @@ -7224,12 +7229,22 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, * numbers) */ if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot reference partitioned table \"%s\"", - RelationGetRelationName(pkrel)))); + { + if (!recurse) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("foreign key referencing partitioned table \"%s\" must not be ONLY", + RelationGetRelationName(pkrel)))); + /* fix recursion in ATExecValidateConstraint to enable this case */ + if (fkconstraint->skip_validation && !fkconstraint->initially_valid) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot add NOT VALID foreign key to relation \"%s\"", + RelationGetRelationName(pkrel)))); + } - if (pkrel->rd_rel->relkind != RELKIND_RELATION) + if (pkrel->rd_rel->relkind != RELKIND_RELATION && + pkrel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("referenced relation \"%s\" is not a table", @@ -7589,6 +7604,45 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, } /* + * If this is a partitioned table, recurse to create the constraint on the + * partitions also. + */ + if (recurse) + { + List *children; + ListCell *child; + + /* XXX why not find_all_inheritors? */ + children = find_inheritance_children(RelationGetRelid(rel), lockmode); + + foreach(child, children) + { + Oid childrelid = lfirst_oid(child); + Relation childrel; + AlteredTableInfo *childtab; + ObjectAddress childAddr; + + /* find_inheritance_children already got lock */ + childrel = heap_open(childrelid, NoLock); + CheckTableNotInUse(childrel, "ALTER TABLE"); /* XXX do we need this? */ + + /* Find or create work queue entry for this table */ + childtab = ATGetQueueEntry(wqueue, childrel); + + /* Recurse to child */ + childAddr = + ATAddForeignKeyConstraint(wqueue, childtab, childrel, + fkconstraint, recurse, true, + lockmode); + + /* make sure they go away together, or not at all */ + recordDependencyOn(&childAddr, &address, DEPENDENCY_INTERNAL); + + heap_close(childrel, NoLock); + } + } + + /* * Close pk table, but keep lock until we've committed. */ heap_close(pkrel, NoLock); @@ -7850,8 +7904,8 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, heap_close(refrel, NoLock); /* - * Foreign keys do not inherit, so we purposely ignore the - * recursion bit here + * We disallow creating invalid foreign keys to or from + * partitioned tables, so ignoring the recursion bit is okay. */ } else if (con->contype == CONSTRAINT_CHECK) @@ -8502,7 +8556,7 @@ CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, * NB: if you change any trigger properties here, see also * ATExecAlterConstraint. */ -static void +void createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, Oid indexOid) { @@ -8632,6 +8686,25 @@ createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, indexOid, true); CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, constraintOid, indexOid, false); + + /* + * If this is a partitioned table, recurse to create triggers for each + * child. We consider that one pg_constraint entry is enough; we only + * need the triggers to appear per-partition. + */ + if (get_rel_relkind(refRelOid) == RELKIND_PARTITIONED_TABLE) + { + ListCell *cell; + List *dchildren; + + /* XXX maybe we need a stronger lock? */ + dchildren = find_inheritance_children(refRelOid, RowShareLock); + foreach(cell, dchildren) + { + createForeignKeyTriggers(rel, lfirst_oid(cell), fkconstraint, + constraintOid, indexOid); + } + } } /* diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index e80edd3668..0a8b7760d1 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -730,12 +730,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column) errmsg("foreign key constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("foreign key constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); /* * Fill in the current attribute's name and throw it into the @@ -849,12 +843,6 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint) errmsg("foreign key constraints are not supported on foreign tables"), parser_errposition(cxt->pstate, constraint->location))); - if (cxt->ispartitioned) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("foreign key constraints are not supported on partitioned tables"), - parser_errposition(cxt->pstate, - constraint->location))); cxt->fkconstraints = lappend(cxt->fkconstraints, constraint); break; diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index b1ae9e5f96..f9c0e62417 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -401,7 +401,7 @@ RI_FKey_check(TriggerData *trigdata) /* ---------- * The query string built is - * SELECT 1 FROM ONLY <pktable> x WHERE pkatt1 = $1 [AND ...] + * SELECT 1 FROM <pktable> x WHERE pkatt1 = $1 [AND ...] * FOR KEY SHARE OF x * The type id's for the $ parameters are those of the * corresponding FK attributes. @@ -409,7 +409,7 @@ RI_FKey_check(TriggerData *trigdata) */ initStringInfo(&querybuf); quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM ONLY %s x", pkrelname); + appendStringInfo(&querybuf, "SELECT 1 FROM %s x", pkrelname); querysep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) { @@ -537,7 +537,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, /* ---------- * The query string built is - * SELECT 1 FROM ONLY <pktable> x WHERE pkatt1 = $1 [AND ...] + * SELECT 1 FROM <pktable> x WHERE pkatt1 = $1 [AND ...] * FOR KEY SHARE OF x * The type id's for the $ parameters are those of the * PK attributes themselves. @@ -545,7 +545,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel, */ initStringInfo(&querybuf); quoteRelationName(pkrelname, pk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM ONLY %s x", pkrelname); + appendStringInfo(&querybuf, "SELECT 1 FROM %s x", pkrelname); querysep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) { @@ -793,7 +793,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action) /* ---------- * The query string built is - * SELECT 1 FROM ONLY <fktable> x WHERE $1 = fkatt1 [AND ...] + * SELECT 1 FROM <fktable> x WHERE $1 = fkatt1 [AND ...] * FOR KEY SHARE OF x * The type id's for the $ parameters are those of the * corresponding PK attributes. @@ -801,7 +801,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action) */ initStringInfo(&querybuf); quoteRelationName(fkrelname, fk_rel); - appendStringInfo(&querybuf, "SELECT 1 FROM ONLY %s x", + appendStringInfo(&querybuf, "SELECT 1 FROM %s x", fkrelname); querysep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) @@ -951,14 +951,14 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS) /* ---------- * The query string built is - * DELETE FROM ONLY <fktable> WHERE $1 = fkatt1 [AND ...] + * DELETE FROM <fktable> WHERE $1 = fkatt1 [AND ...] * The type id's for the $ parameters are those of the * corresponding PK attributes. * ---------- */ initStringInfo(&querybuf); quoteRelationName(fkrelname, fk_rel); - appendStringInfo(&querybuf, "DELETE FROM ONLY %s", fkrelname); + appendStringInfo(&querybuf, "DELETE FROM %s", fkrelname); querysep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) { @@ -1122,7 +1122,7 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS) /* ---------- * The query string built is - * UPDATE ONLY <fktable> SET fkatt1 = $1 [, ...] + * UPDATE <fktable> SET fkatt1 = $1 [, ...] * WHERE $n = fkatt1 [AND ...] * The type id's for the $ parameters are those of the * corresponding PK attributes. Note that we are assuming @@ -1133,7 +1133,7 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS) initStringInfo(&querybuf); initStringInfo(&qualbuf); quoteRelationName(fkrelname, fk_rel); - appendStringInfo(&querybuf, "UPDATE ONLY %s SET", fkrelname); + appendStringInfo(&querybuf, "UPDATE %s SET", fkrelname); querysep = ""; qualsep = "WHERE"; for (i = 0, j = riinfo->nkeys; i < riinfo->nkeys; i++, j++) @@ -1342,7 +1342,7 @@ ri_setnull(TriggerData *trigdata) /* ---------- * The query string built is - * UPDATE ONLY <fktable> SET fkatt1 = NULL [, ...] + * UPDATE <fktable> SET fkatt1 = NULL [, ...] * WHERE $1 = fkatt1 [AND ...] * The type id's for the $ parameters are those of the * corresponding PK attributes. @@ -1351,7 +1351,7 @@ ri_setnull(TriggerData *trigdata) initStringInfo(&querybuf); initStringInfo(&qualbuf); quoteRelationName(fkrelname, fk_rel); - appendStringInfo(&querybuf, "UPDATE ONLY %s SET", fkrelname); + appendStringInfo(&querybuf, "UPDATE %s SET", fkrelname); querysep = ""; qualsep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) @@ -1559,7 +1559,7 @@ ri_setdefault(TriggerData *trigdata) /* ---------- * The query string built is - * UPDATE ONLY <fktable> SET fkatt1 = DEFAULT [, ...] + * UPDATE <fktable> SET fkatt1 = DEFAULT [, ...] * WHERE $1 = fkatt1 [AND ...] * The type id's for the $ parameters are those of the * corresponding PK attributes. @@ -1568,7 +1568,7 @@ ri_setdefault(TriggerData *trigdata) initStringInfo(&querybuf); initStringInfo(&qualbuf); quoteRelationName(fkrelname, fk_rel); - appendStringInfo(&querybuf, "UPDATE ONLY %s SET", fkrelname); + appendStringInfo(&querybuf, "UPDATE %s SET", fkrelname); querysep = ""; qualsep = "WHERE"; for (i = 0; i < riinfo->nkeys; i++) @@ -1895,8 +1895,8 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) /*---------- * The query string built is: - * SELECT fk.keycols FROM ONLY relname fk - * LEFT OUTER JOIN ONLY pkrelname pk + * SELECT fk.keycols FROM relname fk + * LEFT OUTER JOIN pkrelname pk * ON (pk.pkkeycol1=fk.keycol1 [AND ...]) * WHERE pk.pkkeycol1 IS NULL AND * For MATCH SIMPLE: @@ -1922,7 +1922,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) quoteRelationName(pkrelname, pk_rel); quoteRelationName(fkrelname, fk_rel); appendStringInfo(&querybuf, - " FROM ONLY %s fk LEFT OUTER JOIN ONLY %s pk ON", + " FROM %s fk LEFT OUTER JOIN %s pk ON", fkrelname, pkrelname); strcpy(pkattname, "pk."); @@ -2345,22 +2345,6 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation trig_rel, bool rel_is_pk) /* Find or create a hashtable entry for the constraint */ riinfo = ri_LoadConstraintInfo(constraintOid); - /* Do some easy cross-checks against the trigger call data */ - if (rel_is_pk) - { - if (riinfo->fk_relid != trigger->tgconstrrelid || - riinfo->pk_relid != RelationGetRelid(trig_rel)) - elog(ERROR, "wrong pg_constraint entry for trigger \"%s\" on table \"%s\"", - trigger->tgname, RelationGetRelationName(trig_rel)); - } - else - { - if (riinfo->fk_relid != RelationGetRelid(trig_rel) || - riinfo->pk_relid != trigger->tgconstrrelid) - elog(ERROR, "wrong pg_constraint entry for trigger \"%s\" on table \"%s\"", - trigger->tgname, RelationGetRelationName(trig_rel)); - } - return riinfo; } diff --git a/src/include/catalog/pg_constraint_fn.h b/src/include/catalog/pg_constraint_fn.h index 37b0b4ba82..438f5402fe 100644 --- a/src/include/catalog/pg_constraint_fn.h +++ b/src/include/catalog/pg_constraint_fn.h @@ -56,6 +56,8 @@ extern Oid CreateConstraintEntry(const char *constraintName, bool conNoInherit, bool is_internal); +extern void CloneForeignKeyConstraints(Oid parentId, Oid relationId); + extern void RemoveConstraintById(Oid conId); extern void RenameConstraintById(Oid conId, const char *newname); extern void SetValidatedConstraintById(Oid conId); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index da3ff5dbee..c68515345b 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -74,6 +74,10 @@ extern void find_composite_type_dependencies(Oid typeOid, extern void check_of_type(HeapTuple typetuple); +extern void createForeignKeyTriggers(Relation rel, Oid refRelOid, + Constraint *fkconstraint, Oid constraintOid, + Oid indexOid); + extern void register_on_commit_action(Oid relid, OnCommitAction action); extern void remove_on_commit_action(Oid relid); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 2caf930242..65d2058af7 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3290,10 +3290,6 @@ CREATE TABLE partitioned ( a int, b int ) PARTITION BY RANGE (a, (a+b+1)); -ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; -ERROR: foreign key constraints are not supported on partitioned tables -LINE 1: ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; - ^ ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&); ERROR: exclusion constraints are not supported on partitioned tables LINE 1: ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&); diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 866cc99b9f..7c3703b73d 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -276,16 +276,6 @@ CREATE TABLE partitioned ( ) PARTITION BY LIST (a1, a2); -- fail ERROR: cannot use "list" partition strategy with more than one column -- unsupported constraint type for partitioned tables -CREATE TABLE pkrel ( - a int PRIMARY KEY -); -CREATE TABLE partitioned ( - a int REFERENCES pkrel(a) -) PARTITION BY RANGE (a); -ERROR: foreign key constraints are not supported on partitioned tables -LINE 2: a int REFERENCES pkrel(a) - ^ -DROP TABLE pkrel; CREATE TABLE partitioned ( a int, EXCLUDE USING gist (a WITH &&) diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index ed0bb7845b..3f0265b99d 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -375,6 +375,54 @@ DROP TABLE tmp3; DROP TABLE tmp2; +-- Ensure we can add foreign keys to and from partitioned tables +SET search_path TO at_tst; +CREATE SCHEMA at_tst; +CREATE TABLE at_regular1 (col1 INT PRIMARY KEY); +CREATE TABLE at_partitioned (col2 INT PRIMARY KEY, + col1 INT NOT NULL) PARTITION BY RANGE (col2); +CREATE TABLE at_regular2 (col2 INT); +ALTER TABLE at_regular2 ADD FOREIGN KEY (col2) REFERENCES at_partitioned; +ALTER TABLE at_partitioned ADD FOREIGN KEY (col1) REFERENCES at_regular1; +CREATE TABLE at_partitioned_0 PARTITION OF at_partitioned + FOR VALUES FROM (0) TO (10000); +-- these fail: +INSERT INTO at_regular2 VALUES (1000); +INSERT INTO at_partitioned VALUES (1000, 42); + +-- these work: +INSERT INTO at_regular1 VALUES (1000); +INSERT INTO at_partitioned VALUES (42, 1000); +INSERT INTO at_regular2 VALUES (42); + +CREATE TABLE at_partitioned_1 PARTITION OF at_partitioned + FOR VALUES FROM (10000) TO (20000); +CREATE TABLE at_partitioned_2 (col2 INT, col1 INT); +ALTER TABLE at_partitioned ATTACH PARTITION at_partitioned_2 + FOR VALUES FROM (20000) TO (30000); +ALTER TABLE at_partitioned_2 ALTER col2 SET NOT NULL; +ALTER TABLE at_partitioned ATTACH PARTITION at_partitioned_2 + FOR VALUES FROM (20000) TO (30000); + +\d at_regular2 +\d at_partitioned +\d at_partitioned_0 +\d at_partitioned_1 +\d at_partitioned_2 + +INSERT INTO at_partitioned VALUES (5000, 42), (15000, 1042), (25000, 2042); +INSERT INTO at_regular2 VALUES (5000, 42), (15000, 1042), (25000, 2042); + + +ALTER TABLE at_regular2 DROP CONSTRAINT at_regular2_col2_fkey; +ALTER TABLE at_partitioned_0 DROP CONSTRAINT at_partitioned_col1_fkey; +ALTER TABLE at_partitioned DROP CONSTRAINT at_partitioned_col1_fkey; + +\set VERBOSITY terse +DROP SCHEMA at_tst CASCADE; +\set VERBOSITY default +RESET search_path; + -- NOT VALID with plan invalidation -- ensure we don't use a constraint for -- exclusion until validated set constraint_exclusion TO 'partition'; @@ -2016,7 +2064,6 @@ CREATE TABLE partitioned ( a int, b int ) PARTITION BY RANGE (a, (a+b+1)); -ALTER TABLE partitioned ADD FOREIGN KEY (a) REFERENCES blah; ALTER TABLE partitioned ADD EXCLUDE USING gist (a WITH &&); -- cannot drop column that is part of the partition key diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index fefccf21a2..09a634d79d 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -294,14 +294,6 @@ CREATE TABLE partitioned ( ) PARTITION BY LIST (a1, a2); -- fail -- unsupported constraint type for partitioned tables -CREATE TABLE pkrel ( - a int PRIMARY KEY -); -CREATE TABLE partitioned ( - a int REFERENCES pkrel(a) -) PARTITION BY RANGE (a); -DROP TABLE pkrel; - CREATE TABLE partitioned ( a int, EXCLUDE USING gist (a WITH &&) diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 5f19dad03c..c386e33922 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1055,3 +1055,33 @@ alter table fktable2 drop constraint fktable2_f1_fkey; commit; drop table pktable2, fktable2; + + +-- +-- Foreign keys and partitioned tables +-- + +-- Test that it's possible to have a FK from a partitioned table to a regular +-- one +CREATE TABLE pkregular (f1 int primary key); +CREATE TABLE fkpartit (f1 int references pkregular) PARTITION BY RANGE (f1); +CREATE TABLE fkpart1 PARTITION OF fkpartit FOR VALUES FROM (0) TO (1000); +INSERT INTO fkpartit VALUES (500); +INSERT INTO fkpart1 VALUES (500); +INSERT INTO pkregular VALUES (500); +INSERT INTO fkpartit VALUES (500); +INSERT INTO fkpart1 VALUES (500); +DELETE FROM pkregular; +UPDATE pkregular SET f1 = 501; + +ALTER TABLE fkpart1 DROP CONSTRAINT fkpartit_f1_fkey; -- nope +ALTER TABLE fkpartit DROP CONSTRAINT fkpartit_f1_fkey; +\d fkpartit +\d fkpart1 +ALTER TABLE fkpartit ADD CONSTRAINT fkpartit_f1_fkey FOREIGN KEY (f1) REFERENCES pkregular ON DELETE CASCADE; +\d fkpartit +\d fkpart1 +DELETE FROM pkregular; +SELECT * FROM fkpartit; + +-- Test ALTER TABLE .. ATTACH PARTITION to clone constraints -- 2.11.0