On 2019/01/22 8:30, Alvaro Herrera wrote:
> Hi Amit,
>
> Will you please rebase 0002? Please add your proposed tests cases to
> it, too.
Done. See the attached patches for HEAD and PG 11.
Thanks,
Amit
From 432c4551990d0da1c77b6b9523296b0a2a0a5119 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 22 Jan 2019 13:20:54 +0900
Subject: [PATCH] Do not track foreign key inheritance by dependencies
Inheritance information maintained in pg_constraint is enough to
prevent a child constraint to be dropped and for them to be dropped
when the parent constraint is dropped. So, do not create
dependencies between the parent foreign key constraint and its
children.
Also, fix ATAddForeignKeyConstraint to set connoniherit on the parent
constraint correctly.
---
src/backend/catalog/pg_constraint.c | 27 +++++++++++++--------------
src/backend/commands/indexcmds.c | 22 ++++++++++++++++++++--
src/backend/commands/tablecmds.c | 24 ++++++++++++------------
src/test/regress/expected/foreign_key.out | 17 +++++++++++++++--
src/test/regress/sql/foreign_key.sql | 14 +++++++++++++-
5 files changed, 73 insertions(+), 31 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..855d57c65a 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -762,8 +762,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
* ConstraintSetParentConstraint
* Set a partition's constraint as child of its parent table's
*
- * This updates the constraint's pg_constraint row to show it as inherited, and
- * add a dependency to the parent so that it cannot be removed on its own.
+ * This updates the constraint's pg_constraint row to change its inheritance
+ * properties.
*/
void
ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
@@ -772,8 +772,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
Form_pg_constraint constrForm;
HeapTuple tuple,
newtup;
- ObjectAddress depender;
- ObjectAddress referenced;
constrRel = table_open(ConstraintRelationId, RowExclusiveLock);
tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId));
@@ -785,25 +783,26 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
{
constrForm->conislocal = false;
constrForm->coninhcount++;
+
+ /*
+ * An inherited foreign key constraint can never have more than
one
+ * parent, because inheriting foreign keys is only allowed for
+ * partitioning where multiple inheritance is disallowed.
+ */
+ Assert(constrForm->coninhcount == 1);
+
constrForm->conparentid = parentConstrId;
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
-
- ObjectAddressSet(referenced, ConstraintRelationId,
parentConstrId);
- ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
-
- recordDependencyOn(&depender, &referenced,
DEPENDENCY_INTERNAL_AUTO);
}
else
{
constrForm->coninhcount--;
- if (constrForm->coninhcount <= 0)
- constrForm->conislocal = true;
+ /* See the above comment. */
+ Assert(constrForm->coninhcount == 0);
+ constrForm->conislocal = true;
constrForm->conparentid = InvalidOid;
- deleteDependencyRecordsForClass(ConstraintRelationId,
childConstrId,
-
ConstraintRelationId,
-
DEPENDENCY_INTERNAL_AUTO);
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3edc94308e..6c8aa5d149 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -972,8 +972,26 @@ DefineIndex(Oid relationId,
/* Attach index to parent and
we're done. */
IndexSetParentIndex(cldidx,
indexRelationId);
if (createdConstraintId !=
InvalidOid)
-
ConstraintSetParentConstraint(cldConstrOid,
-
createdConstraintId);
+ {
+ ObjectAddress depender;
+ ObjectAddress
referenced;
+
+
ConstraintSetParentConstraint(cldConstrOid,
+
createdConstraintId);
+
+ /*
+ * Need to set an
explicit dependency in this
+ * case unlike other
types of constraints where
+ * the child constraint
gets dropped due to
+ * inheritance
recursion.
+ */
+
ObjectAddressSet(referenced, ConstraintRelationId,
+
createdConstraintId);
+
ObjectAddressSet(depender, ConstraintRelationId,
+
cldConstrOid);
+
recordDependencyOn(&depender, &referenced,
+
DEPENDENCY_INTERNAL_AUTO);
+ }
if
(!cldidx->rd_index->indisvalid)
invalidate_parent =
true;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fbd2d101c1..0040ac63d2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7251,6 +7251,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo
*tab, Relation rel,
bool old_check_ok;
ObjectAddress address;
ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
+ /* Partitioned table's foreign key must always be inheritable. */
+ bool connoinherit = (rel->rd_rel->relkind !=
+
RELKIND_PARTITIONED_TABLE);
/*
* Grab ShareRowExclusiveLock on the pk table, so that someone doesn't
@@ -7616,7 +7619,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo
*tab, Relation rel,
NULL,
true,
/* islocal */
0,
/* inhcount */
- true,
/* isnoinherit */
+
connoinherit,
false); /* is_internal */
ObjectAddressSet(address, ConstraintRelationId, constrOid);
@@ -7812,8 +7815,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
Constraint *fkconstraint;
bool attach_it;
Oid constrOid;
- ObjectAddress parentAddr,
- childAddr;
ListCell *cell;
int i;
@@ -7830,8 +7831,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
continue;
}
- ObjectAddressSet(parentAddr, ConstraintRelationId,
parentConstrOid);
-
DeconstructFkConstraintRow(tuple, &numfks, conkey, confkey,
conpfeqop,
conppeqop, conffeqop);
for (i = 0; i < numfks; i++)
@@ -7986,9 +7985,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
1, false,
true);
subclone = lappend_oid(subclone, constrOid);
- ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
- recordDependencyOn(&childAddr, &parentAddr,
DEPENDENCY_INTERNAL_AUTO);
-
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
@@ -9284,11 +9280,15 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
con = (Form_pg_constraint) GETSTRUCT(copy_tuple);
- /* Right now only CHECK constraints can be inherited */
- if (con->contype != CONSTRAINT_CHECK)
- elog(ERROR, "inherited constraint is not a CHECK
constraint");
+ /*
+ * Only CHECK constraints and foreign key constraints can be
+ * inherited.
+ */
+ if (con->contype != CONSTRAINT_CHECK &&
+ con->contype != CONSTRAINT_FOREIGN)
+ elog(ERROR, "inherited constraint is not a CHECK
constraint or a foreign key constraint");
- if (con->coninhcount <= 0) /* shouldn't happen */
+ if (con->coninhcount == 0) /* shouldn't happen */
elog(ERROR, "relation %u has non-inherited constraint
\"%s\"",
childrelid, constrName);
diff --git a/src/test/regress/expected/foreign_key.out
b/src/test/regress/expected/foreign_key.out
index 76040a7601..ba83c9d9bb 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1945,7 +1945,20 @@ DETAIL: Key (a)=(2) is not present in table "pkey".
delete from fkpart1.pkey where a = 1;
ERROR: update or delete on table "pkey" violates foreign key constraint
"fk_part_a_fkey" on table "fk_part_1"
DETAIL: Key (a)=(1) is still referenced from table "fk_part_1".
+-- verify that attaching and detaching partitions manipulates the inheritance
+-- properties of their FK constraints correctly
+create schema fkpart2;
+create table fkpart2.pkey (a int primary key);
+create table fkpart2.fk_part (a int, constraint p_fkey foreign key (a)
references fkpart2.pkey) partition by list (a);
+create table fkpart2.fk_part_1 partition of fkpart2.fk_part for values in (1)
partition by list (a);
+create table fkpart2.fk_part_1_1 partition of fkpart2.fk_part_1 for values in
(1);
+alter table fkpart2.fk_part_1 drop constraint p_fkey; -- should fail
+ERROR: cannot drop inherited constraint "p_fkey" of relation "fk_part_1"
+alter table fkpart2.fk_part_1_1 drop constraint p_fkey; -- should fail
+ERROR: cannot drop inherited constraint "p_fkey" of relation "fk_part_1_1"
+alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
+alter table fkpart2.fk_part_1 drop constraint p_fkey;
\set VERBOSITY terse \\ -- suppress cascade details
-drop schema fkpart0, fkpart1 cascade;
-NOTICE: drop cascades to 5 other objects
+drop schema fkpart0, fkpart1, fkpart2 cascade;
+NOTICE: drop cascades to 8 other objects
\set VERBOSITY default
diff --git a/src/test/regress/sql/foreign_key.sql
b/src/test/regress/sql/foreign_key.sql
index 9ed1166c66..966162d045 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1392,6 +1392,18 @@ create table fkpart1.fk_part_1_2 partition of
fkpart1.fk_part_1 for values in (2
insert into fkpart1.fk_part_1 values (2); -- should fail
delete from fkpart1.pkey where a = 1;
+-- verify that attaching and detaching partitions manipulates the inheritance
+-- properties of their FK constraints correctly
+create schema fkpart2;
+create table fkpart2.pkey (a int primary key);
+create table fkpart2.fk_part (a int, constraint p_fkey foreign key (a)
references fkpart2.pkey) partition by list (a);
+create table fkpart2.fk_part_1 partition of fkpart2.fk_part for values in (1)
partition by list (a);
+create table fkpart2.fk_part_1_1 partition of fkpart2.fk_part_1 for values in
(1);
+alter table fkpart2.fk_part_1 drop constraint p_fkey; -- should fail
+alter table fkpart2.fk_part_1_1 drop constraint p_fkey; -- should fail
+alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
+alter table fkpart2.fk_part_1 drop constraint p_fkey;
+
\set VERBOSITY terse \\ -- suppress cascade details
-drop schema fkpart0, fkpart1 cascade;
+drop schema fkpart0, fkpart1, fkpart2 cascade;
\set VERBOSITY default
--
2.11.0
From 5178942bc853b3c70ff03750477ae3519d82883f Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 22 Jan 2019 12:48:12 +0900
Subject: [PATCH] Do not track foreign key inheritance by dependencies
Inheritance information maintained in pg_constraint is enough to
prevent a child constraint to be dropped and for them to be dropped
when the parent constraint is dropped. So, do not create
dependencies between the parent foreign key constraint and its
children.
Also, fix ATAddForeignKeyConstraint to set connoniherit on the parent
constraint correctly.
---
src/backend/catalog/pg_constraint.c | 27 +++++++++++++--------------
src/backend/commands/indexcmds.c | 18 ++++++++++++++++++
src/backend/commands/tablecmds.c | 24 ++++++++++++------------
src/test/regress/expected/foreign_key.out | 17 +++++++++++++++--
src/test/regress/sql/foreign_key.sql | 14 +++++++++++++-
5 files changed, 71 insertions(+), 29 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 5720c652b2..3e939e014f 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -771,8 +771,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
* ConstraintSetParentConstraint
* Set a partition's constraint as child of its parent table's
*
- * This updates the constraint's pg_constraint row to show it as inherited, and
- * add a dependency to the parent so that it cannot be removed on its own.
+ * This updates the constraint's pg_constraint row to change its inheritance
+ * properties.
*/
void
ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
@@ -781,8 +781,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
Form_pg_constraint constrForm;
HeapTuple tuple,
newtup;
- ObjectAddress depender;
- ObjectAddress referenced;
constrRel = heap_open(ConstraintRelationId, RowExclusiveLock);
tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId));
@@ -794,25 +792,26 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
{
constrForm->conislocal = false;
constrForm->coninhcount++;
+
+ /*
+ * An inherited foreign key constraint can never have more than
one
+ * parent, because inheriting foreign keys is only allowed for
+ * partitioning where multiple inheritance is disallowed.
+ */
+ Assert(constrForm->coninhcount == 1);
+
constrForm->conparentid = parentConstrId;
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
-
- ObjectAddressSet(referenced, ConstraintRelationId,
parentConstrId);
- ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
-
- recordDependencyOn(&depender, &referenced,
DEPENDENCY_INTERNAL_AUTO);
}
else
{
constrForm->coninhcount--;
- if (constrForm->coninhcount <= 0)
- constrForm->conislocal = true;
+ /* See the above comment. */
+ Assert(constrForm->coninhcount == 0);
+ constrForm->conislocal = true;
constrForm->conparentid = InvalidOid;
- deleteDependencyRecordsForClass(ConstraintRelationId,
childConstrId,
-
ConstraintRelationId,
-
DEPENDENCY_INTERNAL_AUTO);
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fec5bc5dd6..048fabcc61 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -974,9 +974,27 @@ DefineIndex(Oid relationId,
/* Attach index to parent and
we're done. */
IndexSetParentIndex(cldidx,
indexRelationId);
if (createdConstraintId !=
InvalidOid)
+ {
+ ObjectAddress depender;
+ ObjectAddress
referenced;
+
ConstraintSetParentConstraint(cldConstrOid,
createdConstraintId);
+ /*
+ * Need to set an
explicit dependency in this
+ * case unlike other
types of constraints where
+ * the child constraint
gets dropped due to
+ * inheritance
recursion.
+ */
+
ObjectAddressSet(referenced, ConstraintRelationId,
+
createdConstraintId);
+
ObjectAddressSet(depender, ConstraintRelationId,
+
cldConstrOid);
+
recordDependencyOn(&depender, &referenced,
+
DEPENDENCY_INTERNAL_AUTO);
+ }
+
if
(!IndexIsValid(cldidx->rd_index))
invalidate_parent =
true;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5a62adbafc..9ec27eb6bf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7344,6 +7344,9 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo
*tab, Relation rel,
bool old_check_ok;
ObjectAddress address;
ListCell *old_pfeqop_item = list_head(fkconstraint->old_conpfeqop);
+ /* Partitioned table's foreign key must always be inheritable. */
+ bool connoinherit = (rel->rd_rel->relkind !=
+
RELKIND_PARTITIONED_TABLE);
/*
* Grab ShareRowExclusiveLock on the pk table, so that someone doesn't
@@ -7710,7 +7713,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo
*tab, Relation rel,
NULL,
true,
/* islocal */
0,
/* inhcount */
- true,
/* isnoinherit */
+
connoinherit,
false); /* is_internal */
ObjectAddressSet(address, ConstraintRelationId, constrOid);
@@ -7906,8 +7909,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
Constraint *fkconstraint;
bool attach_it;
Oid constrOid;
- ObjectAddress parentAddr,
- childAddr;
ListCell *cell;
int i;
@@ -7924,8 +7925,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
continue;
}
- ObjectAddressSet(parentAddr, ConstraintRelationId,
parentConstrOid);
-
DeconstructFkConstraintRow(tuple, &numfks, conkey, confkey,
conpfeqop,
conppeqop, conffeqop);
for (i = 0; i < numfks; i++)
@@ -8081,9 +8080,6 @@ CloneFkReferencing(Relation pg_constraint, Relation
parentRel,
1, false,
true);
subclone = lappend_oid(subclone, constrOid);
- ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
- recordDependencyOn(&childAddr, &parentAddr,
DEPENDENCY_INTERNAL_AUTO);
-
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
@@ -9383,11 +9379,15 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
con = (Form_pg_constraint) GETSTRUCT(copy_tuple);
- /* Right now only CHECK constraints can be inherited */
- if (con->contype != CONSTRAINT_CHECK)
- elog(ERROR, "inherited constraint is not a CHECK
constraint");
+ /*
+ * Only CHECK constraints and foreign key constraints can be
+ * inherited.
+ */
+ if (con->contype != CONSTRAINT_CHECK &&
+ con->contype != CONSTRAINT_FOREIGN)
+ elog(ERROR, "inherited constraint is not a CHECK
constraint or a foreign key constraint");
- if (con->coninhcount <= 0) /* shouldn't happen */
+ if (con->coninhcount == 0) /* shouldn't happen */
elog(ERROR, "relation %u has non-inherited constraint
\"%s\"",
childrelid, constrName);
diff --git a/src/test/regress/expected/foreign_key.out
b/src/test/regress/expected/foreign_key.out
index 4fd2341388..aa44daf0e8 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1884,7 +1884,20 @@ DETAIL: Key (a)=(2) is not present in table "pkey".
delete from fkpart1.pkey where a = 1;
ERROR: update or delete on table "pkey" violates foreign key constraint
"fk_part_a_fkey" on table "fk_part_1"
DETAIL: Key (a)=(1) is still referenced from table "fk_part_1".
+-- verify that attaching and detaching partitions manipulates the inheritance
+-- properties of their FK constraints correctly
+create schema fkpart2;
+create table fkpart2.pkey (a int primary key);
+create table fkpart2.fk_part (a int, constraint p_fkey foreign key (a)
references fkpart2.pkey) partition by list (a);
+create table fkpart2.fk_part_1 partition of fkpart2.fk_part for values in (1)
partition by list (a);
+create table fkpart2.fk_part_1_1 partition of fkpart2.fk_part_1 for values in
(1);
+alter table fkpart2.fk_part_1 drop constraint p_fkey; -- should fail
+ERROR: cannot drop inherited constraint "p_fkey" of relation "fk_part_1"
+alter table fkpart2.fk_part_1_1 drop constraint p_fkey; -- should fail
+ERROR: cannot drop inherited constraint "p_fkey" of relation "fk_part_1_1"
+alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
+alter table fkpart2.fk_part_1 drop constraint p_fkey;
\set VERBOSITY terse \\ -- suppress cascade details
-drop schema fkpart0, fkpart1 cascade;
-NOTICE: drop cascades to 5 other objects
+drop schema fkpart0, fkpart1, fkpart2 cascade;
+NOTICE: drop cascades to 8 other objects
\set VERBOSITY default
diff --git a/src/test/regress/sql/foreign_key.sql
b/src/test/regress/sql/foreign_key.sql
index dd0be01c77..fe9b300d90 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1341,6 +1341,18 @@ create table fkpart1.fk_part_1_2 partition of
fkpart1.fk_part_1 for values in (2
insert into fkpart1.fk_part_1 values (2); -- should fail
delete from fkpart1.pkey where a = 1;
+-- verify that attaching and detaching partitions manipulates the inheritance
+-- properties of their FK constraints correctly
+create schema fkpart2;
+create table fkpart2.pkey (a int primary key);
+create table fkpart2.fk_part (a int, constraint p_fkey foreign key (a)
references fkpart2.pkey) partition by list (a);
+create table fkpart2.fk_part_1 partition of fkpart2.fk_part for values in (1)
partition by list (a);
+create table fkpart2.fk_part_1_1 partition of fkpart2.fk_part_1 for values in
(1);
+alter table fkpart2.fk_part_1 drop constraint p_fkey; -- should fail
+alter table fkpart2.fk_part_1_1 drop constraint p_fkey; -- should fail
+alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
+alter table fkpart2.fk_part_1 drop constraint p_fkey;
+
\set VERBOSITY terse \\ -- suppress cascade details
-drop schema fkpart0, fkpart1 cascade;
+drop schema fkpart0, fkpart1, fkpart2 cascade;
\set VERBOSITY default
--
2.11.0
