Hi,
On 2019/01/24 6:13, Alvaro Herrera wrote:
> On 2019-Jan-22, Amit Langote wrote:
>> Done. See the attached patches for HEAD and PG 11.
>
> I'm not quite sure I understand why the one in DefineIndex needs the
> deps but nothing else does. I fear that you added that one just to
> appease the existing test that breaks otherwise, and I worry that with
> that addition we're papering over some other, more fundamental bug.
Thinking more on this, my proposal to rip dependencies between parent and
child constraints altogether to resolve the bug I initially reported is
starting to sound a bit overambitious especially considering that we'd
need to back-patch it (the patch didn't even consider index constraints
properly, creating a divergence between the behaviors of inherited foreign
key constraints and inherited index constraints). We can pursue it if
only to avoid bloating the catalog for what can be achieved with little
bit of additional code in tablecmds.c, but maybe we should refrain from
doing it in reaction to this particular bug.
I've updated the patch that implements a much simpler fix for this
particular bug. Just to reiterate, the following illustrates the bug:
create table pk (a int primary key);
create table p (a int references pk) partition by list (a);
create table p1 partition of p for values in (1) partition by list (a);
create table p11 partition of p1 for values in (1);
alter table p detach partition p1;
alter table p1 drop constraint p_a_fkey;
ERROR: constraint "p_a_fkey" of relation "p11" does not exist
The error occurs because ATExecDropConstraint when recursively called on
p11 cannot find the constraint as the dependency mechanism already dropped
it. The new fix is to return from ATExecDropConstraint without recursing
if the constraint being dropped is index or foreign constraint.
A few hunks of the originally proposed patch are attached here as 0001,
especially the part which fixes ATAddForeignKeyConstraint to pass the
correct value of connoninherit to CreateConstraintEntry (which should be
false for partitioned tables). With that change, many tests start failing
because of the above bug. That patch also adds a test case like the one
above, but it fails along with others due to the bug. Patch 0002 is the
aforementioned simpler fix to make the errors (existing and the newly
added) go away.
These patches apply unchanged to the PG 11 branch.
Thanks,
Amit
From 8eb5ce74e8656b517ad5a4e960b70de0ff3bedd5 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 24 Jan 2019 21:29:17 +0900
Subject: [PATCH 1/2] Fix ATAddForeignKeyConstraint to use correct value of
connoinherit
While at it, add some Asserts to ConstraintSetParentConstraint to
assert the correct value of coninhcount.
Many tests fail, but the next patch should take care of them.
---
src/backend/catalog/pg_constraint.c | 11 +++++++++--
src/backend/commands/tablecmds.c | 5 ++++-
src/test/regress/expected/foreign_key.out | 18 ++++++++++++++++--
src/test/regress/sql/foreign_key.sql | 15 ++++++++++++++-
4 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..d2b8489b6c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -785,6 +785,12 @@ 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);
@@ -797,8 +803,9 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
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,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 738c178107..fea4d99735 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, /* isnoinherit */
false); /* is_internal */
ObjectAddressSet(address, ConstraintRelationId, constrOid);
diff --git a/src/test/regress/expected/foreign_key.out
b/src/test/regress/expected/foreign_key.out
index 76040a7601..b50bafef9e 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1945,7 +1945,21 @@ 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 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 (a int, constraint my_fkey foreign key (a)
references fkpart2.pkey);
+alter table fkpart2.fk_part_1 attach partition fkpart2.fk_part_1_1 for values
in (1);
+alter table fkpart2.fk_part_1 drop constraint fkey; -- should fail
+ERROR: cannot drop inherited constraint "fkey" of relation "fk_part_1"
+alter table fkpart2.fk_part_1_1 drop constraint my_fkey; -- should fail
+ERROR: cannot drop inherited constraint "my_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 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..c02251eaa8 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1392,6 +1392,19 @@ 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 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 (a int, constraint my_fkey foreign key (a)
references fkpart2.pkey);
+alter table fkpart2.fk_part_1 attach partition fkpart2.fk_part_1_1 for values
in (1);
+alter table fkpart2.fk_part_1 drop constraint fkey; -- should fail
+alter table fkpart2.fk_part_1_1 drop constraint my_fkey; -- should fail
+alter table fkpart2.fk_part detach partition fkpart2.fk_part_1;
+alter table fkpart2.fk_part_1 drop constraint 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 c2c9bd4c4976902525ebfcb0e32eb8249df549c9 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 24 Jan 2019 20:42:11 +0900
Subject: [PATCH 2/2] Don't recurse in ATExecDropConstraint for non-CHECK
constraints
---
src/backend/commands/tablecmds.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fea4d99735..82e2cf1473 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9134,6 +9134,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
HeapTuple tuple;
bool found = false;
bool is_no_inherit_constraint = false;
+ char contype;
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
@@ -9203,6 +9204,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
performDeletion(&conobj, behavior, 0);
found = true;
+ contype = con->contype;
}
systable_endscan(scan);
@@ -9227,6 +9229,16 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
}
/*
+ * Non-CHECK inherited constraints are dropped via dependency mechanism,
+ * so we're done here for such constraints.
+ */
+ if (contype != CONSTRAINT_CHECK)
+ {
+ table_close(conrel, RowExclusiveLock);
+ return;
+ }
+
+ /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we
can't
* use find_all_inheritors to do it in one pass.
--
2.11.0
