On 2024-Jul-26, Junwang Zhao wrote:
> There is a bug report[0] Tender comments might be the same
> issue as this one, but I tried Alvaro's and mine patch, neither
> could solve that problem, I did not tried Tender's earlier patch
> thought. I post the test script below in case you are interested.
Yeah, I've been looking at this whole debacle this week and after
looking at it more closely, I realized that the overall problem requires
a much more invasive solution -- namely, that on DETACH, if the
referenced table is partitioned, we need to create additional
pg_constraint entries from the now-standalone table (was partition) to
each of the partitions of the referenced table; and also add action
triggers to each of those. Without that, the constraint is incomplete
and doesn't work (as reported multiple times already).
One thing I have not yet tried is what if the partition being detach is
also partitioned. I mean, do we need to handle each sub-partition
explicitly in some way? I think the answer is no, but it needs tests.
I have written the patch to do this on detach, and AFAICS it works well,
though it changes the behavior of some existing tests (IIRC related to
self-referencing FKs). Also, the next problem is making sure that
ATTACH deals with it correctly. I'm on this bit today.
Self-referencing FKs seem to have additional problems :-(
The queries I was talking about are these
\set tables ''''prim.*''',''forign.*''',''''lone''''
select oid, conparentid, contype, conname, conrelid::regclass,
confrelid::regclass, conkey, confkey, conindid::regclass from pg_constraint
where contype = 'f' and (conrelid::regclass::text ~ any (array[:tables]) or
confrelid::regclass::text ~ any (array[:tables])) order by contype, conrelid,
confrelid; select tgconstraint, oid, tgrelid::regclass,
tgconstrrelid::regclass, tgname, tgparentid, tgconstrindid::regclass,
tgfoid::regproc from pg_trigger where tgconstraint in (select oid from
pg_constraint where conrelid::regclass::text ~ any (array[:tables]) or
confrelid::regclass::text ~ any (array[:tables])) order by tgconstraint,
tgrelid::regclass::text, tgfoid;
Written as a single line in psql they let you quickly see all the
constraints and their associated triggers, so for instance you can see
whether this sequence
create table prim (a int primary key) partition by list (a);
create table prim1 partition of prim for values in (1);
create table prim2 partition of prim for values in (2);
create table forign (a int references prim) partition by list (a);
create table forign1 partition of forign for values in (1);
create table forign2 partition of forign for values in (2);
alter table forign detach partition forign1;
produces the same set of constraints and triggers as this other sequence
create table prim (a int primary key) partition by list (a);
create table prim1 partition of prim for values in (1);
create table prim2 partition of prim for values in (2);
create table forign (a int references prim) partition by list (a);
create table forign2 partition of forign for values in (2);
create table forign1 (a int references prim);
The patch is more or less like the attached, far from ready.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 721d24783b..1ea4003aec 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10634,7 +10634,7 @@ CloneFkReferenced(Relation parentRel, Relation
partitionRel)
* 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
* AccessShareLock suffices (assumes that dropping the
constraint
- * acquires AEL).
+ * acquires AccessExclusiveLock).
*/
fkRel = table_open(constrForm->conrelid, AccessShareLock);
@@ -19175,8 +19175,10 @@ DetachPartitionFinalize(Relation rel, Relation
partRel, bool concurrent,
DropClonedTriggersFromPartition(RelationGetRelid(partRel));
/*
- * Detach any foreign keys that are inherited. This includes creating
- * additional action triggers.
+ * Detach any foreign keys that are inherited. This requires marking
some
+ * constraints and triggers as no longer having parents, as well as
+ * creating additional constraint entries and triggers, depending on the
+ * shape of each FK.
*/
fks = copyObject(RelationGetFKeyList(partRel));
if (fks != NIL)
@@ -19185,10 +19187,15 @@ DetachPartitionFinalize(Relation rel, Relation
partRel, bool concurrent,
{
ForeignKeyCacheInfo *fk = lfirst(cell);
HeapTuple contup;
+ HeapTuple parentConTup;
Form_pg_constraint conform;
+ Form_pg_constraint parentConForm;
Constraint *fkconstraint;
- Oid insertTriggerOid,
- updateTriggerOid;
+ Oid parentConstrOid;
+ Oid insertCheckTrigOid,
+ updateCheckTrigOid,
+ updateActionTrigOid,
+ deleteActionTrigOid;
contup = SearchSysCache1(CONSTROID,
ObjectIdGetDatum(fk->conoid));
if (!HeapTupleIsValid(contup))
@@ -19203,28 +19210,34 @@ DetachPartitionFinalize(Relation rel, Relation
partRel, bool concurrent,
continue;
}
- /* unset conparentid and adjust conislocal, coninhcount, etc. */
- ConstraintSetParentConstraint(fk->conoid, InvalidOid,
InvalidOid);
-
/*
- * Also, look up the partition's "check" triggers corresponding
to the
- * constraint being detached and detach them from the parent
triggers.
+ * To create a FK constraint in the to-be-detached partition
+ * equivalent to what exists in the parent table, what to do
depends
+ * on whether the referenced table is partitioned or not. If it
+ * isn't, then just reparenting the pg_constraint row and
pg_trigger
+ * rows for the existing constraint, and creating new action
triggers,
+ * is sufficient. However, if the referenced table is
partitioned,
+ * then we must create additional pg_constraint rows -- one for
each
+ * partition -- and the necessary action triggers for each of
them.
*/
- GetForeignKeyCheckTriggers(trigrel,
- fk->conoid,
fk->confrelid, fk->conrelid,
-
&insertTriggerOid, &updateTriggerOid);
- Assert(OidIsValid(insertTriggerOid));
- TriggerSetParentTrigger(trigrel, insertTriggerOid, InvalidOid,
-
RelationGetRelid(partRel));
- Assert(OidIsValid(updateTriggerOid));
- TriggerSetParentTrigger(trigrel, updateTriggerOid, InvalidOid,
-
RelationGetRelid(partRel));
+ parentConstrOid = conform->conparentid;
- /*
- * Make the action triggers on the referenced relation. When
this was
- * a partition the action triggers pointed to the parent rel
(they
- * still do), but now we need separate ones of our own.
- */
+ Assert(OidIsValid(conform->conparentid));
+ parentConTup = SearchSysCache1(CONSTROID,
+
ObjectIdGetDatum(parentConstrOid));
+ if (!HeapTupleIsValid(parentConTup))
+ elog(ERROR, "cache lookup failed for constraint %u",
+ conform->conparentid);
+ parentConForm = (Form_pg_constraint) GETSTRUCT(parentConTup);
+
+ if (parentConForm->confrelid == RelationGetRelid(rel))
+ {
+ elog(WARNING, "detaching self-referencing FK %u (parent
%u) from %s to %s",
+ conform->oid, conform->conparentid,
+ RelationGetRelationName(partRel),
RelationGetRelationName(rel));
+ }
+
+ /* Create a node we'll use throughout this */
fkconstraint = makeNode(Constraint);
fkconstraint->contype = CONSTRAINT_FOREIGN;
fkconstraint->conname = pstrdup(NameStr(conform->conname));
@@ -19240,15 +19253,178 @@ DetachPartitionFinalize(Relation rel, Relation
partRel, bool concurrent,
fkconstraint->fk_del_set_cols = NIL;
fkconstraint->old_conpfeqop = NIL;
fkconstraint->old_pktable_oid = InvalidOid;
- fkconstraint->skip_validation = false;
+ fkconstraint->skip_validation = true; /* XXX hmmm */
fkconstraint->initially_valid = true;
+ /*
+ * The constraint on this table must be marked no longer a
child of
+ * the parent's constraint; and its check triggers too.
+ */
+ ConstraintSetParentConstraint(fk->conoid, InvalidOid,
InvalidOid);
+
+ GetForeignKeyCheckTriggers(trigrel,
+ fk->conoid,
fk->confrelid, fk->conrelid,
+
&insertCheckTrigOid, &updateCheckTrigOid);
+ Assert(OidIsValid(insertCheckTrigOid));
+ TriggerSetParentTrigger(trigrel, insertCheckTrigOid, InvalidOid,
+
RelationGetRelid(partRel));
+ Assert(OidIsValid(updateCheckTrigOid));
+ TriggerSetParentTrigger(trigrel, updateCheckTrigOid, InvalidOid,
+
RelationGetRelid(partRel));
+
+ /*
+ * Now create the action triggers in this table that point to
the
+ * referenced table.
+ */
createForeignKeyActionTriggers(partRel, conform->confrelid,
fkconstraint, fk->conoid,
conform->conindid,
InvalidOid, InvalidOid,
-
NULL, NULL);
+
&deleteActionTrigOid, &updateActionTrigOid);
+ /*
+ * If the referenced side is partitioned (which we know because
our
+ * parent's constraint points to a different relation than
ours) then
+ * we must, in addition to the above, create pg_constraint rows
that
+ * point to each partition, each with its own action triggers.
+ */
+ if (parentConForm->conrelid != conform->conrelid)
+ {
+ List *children;
+ ListCell *child;
+ Relation refdRel;
+ Oid indexOid;
+ int numfks;
+ 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];
+ int numfkdelsetcols;
+ AttrNumber confdelsetcols[INDEX_MAX_KEYS];
+ List *colnames = NIL;
+ char *fkaddition;
+
+ DeconstructFkConstraintRow(contup,
+
&numfks,
+
conkey,
+
confkey,
+
conpfeqop,
+
conppeqop,
+
conffeqop,
+
&numfkdelsetcols,
+
confdelsetcols);
+
+ for (int i = 0; i < numfks; i++)
+ {
+ Form_pg_attribute att;
+
+ att = TupleDescAttr(RelationGetDescr(partRel),
+
conkey[i] - 1);
+ colnames = lappend(colnames,
makeString(NameStr(att->attname)));
+ }
+ fkaddition =
ChooseForeignKeyConstraintNameAddition(colnames);
+
+ /*
+ * We're not expanding nor shrinking key space, so
AccessShareLock
+ * is sufficient here given that dropping a constraint
requires
+ * AccessExclusiveLock.
+ */
+ refdRel = table_open(fk->confrelid, AccessShareLock);
+
+ /*
+ * When the referenced table is partitioned, we need new
+ * pg_constraint entries that point from our partition
to each
+ * partition of the other table, and also the action
triggers on
+ * each.
+ */
+ children = find_all_inheritors(fk->confrelid, NoLock,
NULL);
+ foreach(child, children)
+ {
+ Oid childoid =
lfirst_oid(child);
+ Relation fchild;
+ AttrNumber mapped_confkey[INDEX_MAX_KEYS];
+ AttrMap *attmap;
+ char *conname;
+ Oid constrOid;
+ ObjectAddress address,
+ referenced;
+
+ /* A constraint for the topmost parent already
exists */
+ if (childoid == fk->confrelid)
+ continue;
+
+ fchild = table_open(childoid, AccessShareLock);
+
+ attmap =
build_attrmap_by_name(RelationGetDescr(refdRel),
+
RelationGetDescr(fchild),
+
false);
+ for (int i = 0; i < numfks; i++)
+ mapped_confkey[i] =
attmap->attnums[confkey[i] - 1];
+
+ conname =
ChooseConstraintName(RelationGetRelationName(partRel),
+
fkaddition, "fkey",
+
RelationGetNamespace(partRel), NIL);
+
+ indexOid = index_get_partition(fchild,
conform->conindid);
+
+ constrOid =
+ CreateConstraintEntry(conname,
+
RelationGetNamespace(partRel),
+
CONSTRAINT_FOREIGN,
+
fkconstraint->deferrable,
+
fkconstraint->initdeferred,
+
fkconstraint->initially_valid,
+
fk->conoid,
+
RelationGetRelid(partRel),
+
mapped_confkey,
+
numfks,
+
numfks,
+
InvalidOid,
+
indexOid,
+
childoid,
+
conkey,
+
conpfeqop,
+
conppeqop,
+
conffeqop,
+
numfks,
+
fkconstraint->fk_upd_action,
+
fkconstraint->fk_del_action,
+
confdelsetcols,
+
numfkdelsetcols,
+
fkconstraint->fk_matchtype,
+
NULL,
+
NULL,
+
NULL,
+
false,
+
1,
+
false,
+
false);
+
+ /*
+ * Give this constraint partition-type
dependencies on the
+ * parent constraint as well as the table.
+ */
+ ObjectAddressSet(address, ConstraintRelationId,
constrOid);
+ ObjectAddressSet(referenced,
ConstraintRelationId, fk->conoid);
+ recordDependencyOn(&address, &referenced,
DEPENDENCY_PARTITION_PRI);
+ ObjectAddressSet(referenced,
RelationRelationId, fk->conrelid);
+ recordDependencyOn(&address, &referenced,
DEPENDENCY_PARTITION_SEC);
+
+ /* And now create the action triggers that go
with it */
+ createForeignKeyActionTriggers(partRel,
childoid,
+
fkconstraint,
+
constrOid, indexOid,
+
deleteActionTrigOid, updateActionTrigOid,
+
NULL, NULL);
+
+ table_close(fchild, NoLock);
+ }
+
+ table_close(refdRel, NoLock);
+ }
+
+ ReleaseSysCache(parentConTup);
ReleaseSysCache(contup);
}
list_free_deep(fks);
