On 2019-Jan-18, Amit Langote wrote:
> OK, I agree. I have updated the patch to make things work that way.
Thanks, this is better. There were a few other things I didn't like, so
I updated it. Mostly, two things:
1. I didn't like a seqscan on pg_trigger, so I turned that into an
indexed scan on the constraint OID, and then the other two conditions
are checked in the returned tuples. Also, what's the point on
duplicating code and checking how many you deleted? Just delete them
all.
2. I didn't like the ABI break, and it wasn't necessary: you can just
call createForeignKeyActionTriggers directly. That's much simpler.
I also added tests. While running them, I noticed that my previous
commit was broken in terms of relcache invalidation. I don't really
know if this is a new problem with that commit, or an existing one. The
fix is 0001.
Haven't got around to your 0002 yet.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2047f054facf3ffaff31e36591279b9bd76ca53c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 18 Jan 2019 19:06:40 -0300
Subject: [PATCH 1/2] Add missing relcache reset
Adding a foreign key to a partitioned table requires applying a relcache
reset to each partition, so that the newly added FK is correctly
recorded in its entry on next rebuild.
With the previous coding of ATAddForeignKeyConstraint, this may not have
been necessary, but with the one after 0325d7a5957b, it is.
Noted while testing another bugfix.
---
src/backend/commands/tablecmds.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1a1ac698e5..5a27f64aa7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7700,6 +7700,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
childtab->constraints = lappend(childtab->constraints, newcon);
+ /*
+ * Also invalidate the partition's relcache entry, so that its
+ * FK list gets updated on next rebuild.
+ */
+ CacheInvalidateRelcache(partition);
+
heap_close(partition, lockmode);
}
}
--
2.11.0
>From 42b96db12aafea22285893e1d96c63a7f684abfd Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 18 Jan 2019 19:10:29 -0300
Subject: [PATCH 2/2] Fix action triggers for FK constraints on detached
partitions
---
src/backend/commands/tablecmds.c | 72 +++++++++++++++++++++++++++++++++++-
src/test/regress/sql/foreign_key.sql | 20 +++++++++-
2 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5a27f64aa7..02a55ed3d6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7858,6 +7858,10 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
ForeignKeyCacheInfo *fk = lfirst_node(ForeignKeyCacheInfo, cell);
Form_pg_constraint partConstr;
HeapTuple partcontup;
+ Relation trigrel;
+ HeapTuple trigtup;
+ SysScanDesc scan;
+ ScanKeyData key;
attach_it = true;
@@ -7909,7 +7913,39 @@ CloneFkReferencing(Relation pg_constraint, Relation parentRel,
ReleaseSysCache(partcontup);
- /* looks good! Attach this constraint */
+ /*
+ * Looks good! Attach this constraint. Note that the action
+ * triggers are no longer needed, so remove them. We identify
+ * them because they have our constraint OID, as well as being
+ * on the referenced rel.
+ */
+ trigrel = heap_open(TriggerRelationId, RowExclusiveLock);
+ ScanKeyInit(&key,
+ Anum_pg_trigger_tgconstraint,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(fk->conoid));
+
+ scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
+ NULL, 1, &key);
+ while ((trigtup = systable_getnext(scan)) != NULL)
+ {
+ Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
+
+ if (trgform->tgconstrrelid != fk->conrelid)
+ continue;
+ if (trgform->tgrelid != fk->confrelid)
+ continue;
+
+ deleteDependencyRecordsForClass(TriggerRelationId,
+ trgform->oid,
+ ConstraintRelationId,
+ DEPENDENCY_INTERNAL);
+ CatalogTupleDelete(trigrel, &trigtup->t_self);
+ }
+
+ systable_endscan(scan);
+ heap_close(trigrel, RowExclusiveLock);
+
ConstraintSetParentConstraint(fk->conoid, parentConstrOid);
CommandCounterIncrement();
attach_it = true;
@@ -15080,19 +15116,51 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
}
heap_close(classRel, RowExclusiveLock);
- /* Detach foreign keys */
+ /*
+ * Detach any foreign keys that are inherited. This includes creating
+ * additional action triggers.
+ */
fks = copyObject(RelationGetFKeyList(partRel));
foreach(cell, fks)
{
ForeignKeyCacheInfo *fk = lfirst(cell);
HeapTuple contup;
+ Form_pg_constraint conform;
+ Constraint *fkconstraint;
contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid));
if (!contup)
elog(ERROR, "cache lookup failed for constraint %u", fk->conoid);
+ conform = (Form_pg_constraint) GETSTRUCT(contup);
+ /* consider only the inherited foreign keys */
+ if (conform->contype != CONSTRAINT_FOREIGN ||
+ !OidIsValid(conform->conparentid))
+ {
+ ReleaseSysCache(contup);
+ continue;
+ }
+
+ /* unset conparentid and adjust conislocal, coninhcount, etc. */
ConstraintSetParentConstraint(fk->conoid, InvalidOid);
+ /*
+ * 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.
+ */
+ fkconstraint = makeNode(Constraint);
+ fkconstraint->conname = pstrdup(NameStr(conform->conname));
+ fkconstraint->fk_upd_action = conform->confupdtype;
+ fkconstraint->fk_del_action = conform->confdeltype;
+ fkconstraint->deferrable = conform->condeferrable;
+ fkconstraint->initdeferred = conform->condeferred;
+
+ createForeignKeyActionTriggers(partRel, conform->confrelid,
+ fkconstraint, fk->conoid,
+ conform->conindid);
+ CommandCounterIncrement();
+
ReleaseSysCache(contup);
}
list_free_deep(fks);
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index c3a7cfcc01..e08b6a21b0 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1375,6 +1375,24 @@ create table fkpart0.fk_part_56_5 partition of fkpart0.fk_part_56
alter table fkpart0.fk_part_56 drop constraint fk_part_a_fkey;
alter table fkpart0.fk_part_56_5 drop constraint fk_part_a_fkey;
+-- verify that attaching and detaching partitions maintains the right set of
+-- triggers
+create schema fkpart1;
+set search_path to fkpart1;
+ create table pkey (a int primary key);
+ create table fk_part (a int) partition by list (a);
+ create table fk_part_1 partition of fk_part for values in (1) partition by list (a);
+ create table fk_part_1_1 partition of fk_part_1 for values in (1);
+alter table fkpart1.fk_part add foreign key (a) references fkpart1.pkey;
+insert into fkpart1.fk_part values (1); -- should fail
+insert into fkpart1.pkey values (1);
+insert into fkpart1.fk_part values (1);
+delete from fkpart1.pkey where a = 1; -- should fail
+alter table fkpart1.fk_part detach partition fkpart1.fk_part_1;
+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;
+
\set VERBOSITY terse \\ -- suppress cascade details
-drop schema fkpart0 cascade;
+drop schema fkpart0, fkpart1 cascade;
\set VERBOSITY default
--
2.11.0
