Hi Alvaro,
On 2019/03/22 6:54, Alvaro Herrera wrote:
> Here's v7;
Needs rebasing on top of 940311e4bb3.
0001:
+ Oid objectClass = getObjectClass(thisobj);
I guess you meant to use ObjectClass, not Oid here.
Tested 0002 a bit more and found some problems.
create table p (a int primary key) 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);
create table q (a int) partition by list (a);
create table q1 partition of q for values in (1) partition by list (a);
create table q11 partition of q1 for values in (1);
alter table q add foreign key (a) references p;
create table p2 partition of p for values in (2);
ERROR: index for 0 not found in partition p2
The problem appears to be that CloneFkReferenced() is stumbling across
inconsistent pg_constraint rows previously created by
addFkRecurseReferencing() when we did alter table q add foreign key:
select oid, conname, conrelid::regclass, confrelid::regclass, conindid,
conparentid from pg_constraint where conname like '%fkey%';
oid │ conname │ conrelid │ confrelid │ conindid │ conparentid
───────┼───────────┼──────────┼───────────┼──────────┼─────────────
60336 │ q_a_fkey │ q │ p │ 60315 │ 0
60337 │ q_a_fkey1 │ q │ p1 │ 60320 │ 60336
60338 │ q_a_fkey2 │ q │ p11 │ 60325 │ 60337
60341 │ q_a_fkey │ q1 │ p │ 0 │ 60336 <=
60342 │ q_a_fkey │ q11 │ p │ 0 │ 60341 <=
(5 rows)
I think the last two rows contain invalid value of conindid, perhaps as a
result of a bug of addFkRecurseReferencing(). There's this code in it to
fetch the index partition of the referencing partition:
+ /*
+ * No luck finding a good constraint to reuse; create our own.
+ */
+ partIndexOid = index_get_partition(partition, indexOid);
That seems unnecessary. Maybe, it's a remnant of the code copied from
addFkRecurseReferenced(), where using the partition index is necessary
(although without the elog for when we get an InvalidOid, which would've
caught this.) The above index_get_partition() returns InvalidOid,
because, obviously, we don't require referencing side to have the index.
Attached a fix (addFkRecurseReferencing-fix.patch).
Once we apply the above fix, we run into another problem:
create table p2 partition of p for values in (2);
select oid, conname, conrelid::regclass, confrelid::regclass, conindid,
conparentid from pg_constraint where conname like '%fkey%';
oid │ conname │ conrelid │ confrelid │ conindid │ conparentid
───────┼────────────┼──────────┼───────────┼──────────┼─────────────
60336 │ q_a_fkey │ q │ p │ 60315 │ 0
60337 │ q_a_fkey1 │ q │ p1 │ 60320 │ 60336
60338 │ q_a_fkey2 │ q │ p11 │ 60325 │ 60337
60341 │ q_a_fkey │ q1 │ p │ 60315 │ 60336
60342 │ q_a_fkey │ q11 │ p │ 60315 │ 60341
60350 │ q_a_fkey3 │ q │ p2 │ 60348 │ 60336 <=
60353 │ q11_a_fkey │ q11 │ p2 │ 60348 │ 60342 <=
(7 rows)
There are 2 pg_constraint rows both targeting p2, whereas there should've
been only 1 (that is, one for q referencing p2). However, if one adds the
foreign constraint on q *after* creating p2, there is only 1 row, which is
correct.
alter table q drop constraint q_a_fkey;
alter table q add foreign key (a) references p;
select oid, conname, conrelid::regclass, confrelid::regclass, conindid,
conparentid from pg_constraint where conname like '%fkey%';
oid │ conname │ conrelid │ confrelid │ conindid │ conparentid
───────┼───────────┼──────────┼───────────┼──────────┼─────────────
60356 │ q_a_fkey │ q │ p │ 60315 │ 0
60357 │ q_a_fkey1 │ q │ p1 │ 60320 │ 60356
60358 │ q_a_fkey2 │ q │ p11 │ 60325 │ 60357
60361 │ q_a_fkey3 │ q │ p2 │ 60348 │ 60356 <=
60364 │ q_a_fkey │ q1 │ p │ 60315 │ 60356
60365 │ q_a_fkey │ q11 │ p │ 60315 │ 60364
(6 rows)
This appears to be a bug of CloneFkReferenced(). Specifically, the way it
builds the list of foreign key constraint to be cloned. Attached a fix
(CloneFkReferenced-fix.patch).
> As I said before, I'm thinking of getting rid of the whole business of
> checking partitions on the referenced side of an FK at DROP time, and
> instead jut forbid the DROP completely if any FKs reference an ancestor
> of that partition.
Will that allow `DROP TABLE parted_pk CASCADE` to succeed even if
partitions still contain referenced data? I suppose that's the example
you cited upthread as a bug that remains to be solved.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e26053a5c..2ad30c8452 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8105,7 +8105,6 @@ addFkRecurseReferencing(List **wqueue, Constraint
*fkconstraint, Relation rel,
AttrNumber *attmap;
AttrNumber mapped_fkattnum[INDEX_MAX_KEYS];
bool attached;
- Oid partIndexOid;
char *conname;
Oid constrOid;
ObjectAddress address,
@@ -8148,7 +8147,6 @@ addFkRecurseReferencing(List **wqueue, Constraint
*fkconstraint, Relation rel,
/*
* No luck finding a good constraint to reuse; create
our own.
*/
- partIndexOid = index_get_partition(partition, indexOid);
if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
RelationGetRelid(partition),
fkconstraint->conname))
@@ -8171,7 +8169,7 @@ addFkRecurseReferencing(List **wqueue, Constraint
*fkconstraint, Relation rel,
numfks,
numfks,
InvalidOid,
-
partIndexOid,
+
indexOid,
RelationGetRelid(pkrel),
pkattnum,
pfeqoperators,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8e26053a5c..2ad30c8452 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8319,8 +8317,8 @@ CloneFkReferenced(Relation parentRel, Relation
partitionRel,
{
Form_pg_constraint constrForm = (Form_pg_constraint)
GETSTRUCT(tuple);
- /* ignore this constraint if the parent is already on the list
*/
- if (list_member_oid(clone, constrForm->conparentid))
+ /* Only try to clone the top-level constraint; skip child ones.
*/
+ if (constrForm->conparentid != InvalidOid)
continue;
clone = lappend_oid(clone, constrForm->oid);
@@ -8348,13 +8346,6 @@ CloneFkReferenced(Relation parentRel, Relation
partitionRel,
elog(ERROR, "cache lookup failed for constraint %u",
constrOid);
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
- /* skip children whose parents are going to be cloned, as above
*/
- if (list_member_oid(clone, constrForm->conparentid))
- {
- ReleaseSysCache(tuple);
- continue;
- }
-
/*
* Because we're only expanding the key space at the referenced
side,
* we don't need to prevent any operation in the referencing
table, so
