pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: ALTER CONSTRAINT on a partitioned FK isn't working
Date: Thu, 29 Apr 2021 19:02:28 -0400
Message-ID: <20210429230228.GA25949@alvherre.pgsql> (raw )
In-Reply-To: <3144850.1607369633@sss.pgh.pa.us >
On 2020-Dec-07, Tom Lane wrote:
> then everything behaves as expected. So something is broken
> about propagating deferred-ness to partition triggers in an
> ALTER CONSTRAINT. Oddly, it *looks* like it worked if you
> examine the child tables with "\d+". I surmise that ALTER CONSTRAINT
> fixes whatever catalog fields psql looks at, but there's some other
> fields that also need to be updated and aren't being.
I came up with this. As I mentioned in my earlier reply, handling
recursion in the usual way doesn't fix the whole problem, because we
need to recurse on possibly both sides of the constraint. So I made it
recurse using pg_constraint.conparentid, which seems easier and cover
both ends. This is pretty crude so far but it handles the trivial
cases. What I did is split the existing routine in two, and the "inner"
part now recurses on itself if it sees that either table is partitioned.
I'll polish it tomorrow -- intend to get this pushed to branches back to
11, depending on what's needed.
--
Álvaro Herrera Valdivia, Chile
Attachments:
constraints.patch (text/x-diff, 13.2K) download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d9ba87a2a3..deeafb44e4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -355,8 +355,10 @@ static void AlterIndexNamespaces(Relation classRel, Relation rel,
static void AlterSeqNamespaces(Relation classRel, Relation rel,
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
LOCKMODE lockmode);
-static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
- bool recurse, bool recursing, LOCKMODE lockmode);
+static ObjectAddress ATExecAlterConstraint(List **wqueue, AlteredTableInfo *tab,
+ Relation rel, AlterTableCmd *cmd,
+ bool recurse, bool recursing,
+ LOCKMODE lockmode);
static ObjectAddress ATExecValidateConstraint(List **wqueue,
Relation rel, char *constrName,
bool recurse, bool recursing, LOCKMODE lockmode);
@@ -4967,7 +4969,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
lockmode);
break;
case AT_AlterConstraint: /* ALTER CONSTRAINT */
- address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
+ address = ATExecAlterConstraint(wqueue, tab, rel, cmd, false, false, lockmode);
break;
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
address = ATExecValidateConstraint(wqueue, rel, cmd->name, false,
@@ -10190,16 +10192,18 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk,
* Update the attributes of a constraint.
*
* Currently only works for Foreign Key constraints.
- * Foreign keys do not inherit, so we purposely ignore the
- * recursion bit here, but we keep the API the same for when
- * other constraint types are supported.
*
* If the constraint is modified, returns its address; otherwise, return
* InvalidObjectAddress.
*/
+static void
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, Relation rel,
+ HeapTuple contuple, List **otherrelids,
+ LOCKMODE lockmode);
static ObjectAddress
-ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
- bool recurse, bool recursing, LOCKMODE lockmode)
+ATExecAlterConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
+ AlterTableCmd *cmd, bool recurse, bool recursing,
+ LOCKMODE lockmode)
{
Constraint *cmdcon;
Relation conrel;
@@ -10208,6 +10212,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
HeapTuple contuple;
Form_pg_constraint currcon;
ObjectAddress address;
+ List *otherrelids = NIL;
+ ListCell *lc;
cmdcon = castNode(Constraint, cmd->def);
@@ -10248,103 +10254,30 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
if (currcon->condeferrable != cmdcon->deferrable ||
currcon->condeferred != cmdcon->initdeferred)
{
- HeapTuple copyTuple;
- HeapTuple tgtuple;
- Form_pg_constraint copy_con;
- List *otherrelids = NIL;
- ScanKeyData tgkey;
- SysScanDesc tgscan;
Relation tgrel;
- ListCell *lc;
- /*
- * Now update the catalog, while we have the door open.
- */
- copyTuple = heap_copytuple(contuple);
- copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
- copy_con->condeferrable = cmdcon->deferrable;
- copy_con->condeferred = cmdcon->initdeferred;
- CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple);
-
- InvokeObjectPostAlterHook(ConstraintRelationId,
- currcon->oid, 0);
-
- heap_freetuple(copyTuple);
-
- /*
- * Now we need to update the multiple entries in pg_trigger that
- * implement the constraint.
- */
tgrel = table_open(TriggerRelationId, RowExclusiveLock);
-
- ScanKeyInit(&tgkey,
- Anum_pg_trigger_tgconstraint,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(currcon->oid));
-
- tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
- NULL, 1, &tgkey);
-
- while (HeapTupleIsValid(tgtuple = systable_getnext(tgscan)))
- {
- Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
- Form_pg_trigger copy_tg;
-
- /*
- * Remember OIDs of other relation(s) involved in FK constraint.
- * (Note: it's likely that we could skip forcing a relcache inval
- * for other rels that don't have a trigger whose properties
- * change, but let's be conservative.)
- */
- if (tgform->tgrelid != RelationGetRelid(rel))
- otherrelids = list_append_unique_oid(otherrelids,
- tgform->tgrelid);
-
- /*
- * Update deferrability of RI_FKey_noaction_del,
- * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
- * triggers, but not others; see createForeignKeyActionTriggers
- * and CreateFKCheckTrigger.
- */
- if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
- tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
- tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
- tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
- continue;
-
- copyTuple = heap_copytuple(tgtuple);
- copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
-
- copy_tg->tgdeferrable = cmdcon->deferrable;
- copy_tg->tginitdeferred = cmdcon->initdeferred;
- CatalogTupleUpdate(tgrel, ©Tuple->t_self, copyTuple);
-
- InvokeObjectPostAlterHook(TriggerRelationId, currcon->oid, 0);
-
- heap_freetuple(copyTuple);
- }
-
- systable_endscan(tgscan);
+ ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, rel, contuple,
+ &otherrelids, lockmode);
+ ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
table_close(tgrel, RowExclusiveLock);
-
- /*
- * Invalidate relcache so that others see the new attributes. We must
- * inval both the named rel and any others having relevant triggers.
- * (At present there should always be exactly one other rel, but
- * there's no need to hard-wire such an assumption here.)
- */
- CacheInvalidateRelcache(rel);
- foreach(lc, otherrelids)
- {
- CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
- }
-
- ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
}
else
address = InvalidObjectAddress;
+ /*
+ * Invalidate relcache so that others see the new attributes. We must
+ * inval both the named rel and any others having relevant triggers.
+ * (At present there should always be exactly one other rel, but
+ * there's no need to hard-wire such an assumption here.)
+ */
+ CacheInvalidateRelcache(rel);
+ foreach(lc, otherrelids)
+ {
+ CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+ }
+
systable_endscan(scan);
table_close(conrel, RowExclusiveLock);
@@ -10352,6 +10285,131 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
return address;
}
+/*
+ * Do the actual job of updating pg_constraint and pg_trigger tuples for
+ * ALTER TABLE ALTER CONSTRAINT. This is split out of ATExecAlterConstraint
+ * because we need to recurse to children when processing partitioned tables.
+ */
+static void
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, Relation rel,
+ HeapTuple contuple, List **otherrelids,
+ LOCKMODE lockmode)
+{
+ Form_pg_constraint currcon;
+ Form_pg_constraint copy_con;
+ HeapTuple copyTuple;
+ HeapTuple tgtuple;
+ ScanKeyData tgkey;
+ SysScanDesc tgscan;
+ Oid conoid;
+ Oid refrelid;
+
+ /*
+ * Now update the catalog, while we have the door open.
+ */
+ currcon = (Form_pg_constraint) GETSTRUCT(contuple);
+ refrelid = currcon->confrelid;
+ conoid = currcon->oid;
+ copyTuple = heap_copytuple(contuple);
+ copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
+ copy_con->condeferrable = cmdcon->deferrable;
+ copy_con->condeferred = cmdcon->initdeferred;
+ CatalogTupleUpdate(conrel, ©Tuple->t_self, copyTuple);
+
+ InvokeObjectPostAlterHook(ConstraintRelationId,
+ conoid, 0);
+
+ heap_freetuple(copyTuple);
+
+ /*
+ * Now we need to update the multiple entries in pg_trigger that
+ * implement the constraint.
+ */
+ ScanKeyInit(&tgkey,
+ Anum_pg_trigger_tgconstraint,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(conoid));
+
+ tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
+ NULL, 1, &tgkey);
+
+ while (HeapTupleIsValid(tgtuple = systable_getnext(tgscan)))
+ {
+ Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
+ Form_pg_trigger copy_tg;
+
+ /*
+ * Remember OIDs of other relation(s) involved in FK constraint.
+ * (Note: it's likely that we could skip forcing a relcache inval
+ * for other rels that don't have a trigger whose properties
+ * change, but let's be conservative.)
+ */
+ if (tgform->tgrelid != RelationGetRelid(rel))
+ *otherrelids = list_append_unique_oid(*otherrelids,
+ tgform->tgrelid);
+
+ /*
+ * Update deferrability of RI_FKey_noaction_del,
+ * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
+ * triggers, but not others; see createForeignKeyActionTriggers
+ * and CreateFKCheckTrigger.
+ */
+ if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
+ tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
+ tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
+ tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
+ continue;
+
+ copyTuple = heap_copytuple(tgtuple);
+ copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
+
+ copy_tg->tgdeferrable = cmdcon->deferrable;
+ copy_tg->tginitdeferred = cmdcon->initdeferred;
+ CatalogTupleUpdate(tgrel, ©Tuple->t_self, copyTuple);
+
+ InvokeObjectPostAlterHook(TriggerRelationId, conoid, 0);
+
+ heap_freetuple(copyTuple);
+ }
+
+ systable_endscan(tgscan);
+
+ /*
+ * If the table that the partition is on is a partitioned table, we need
+ * to recurse and handle every constraint that is a child of this
+ * constraint.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+ get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)
+ {
+ ScanKeyData pkey;
+ SysScanDesc pscan;
+ HeapTuple childtup;
+
+ ScanKeyInit(&pkey,
+ Anum_pg_constraint_conparentid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(conoid));
+
+ pscan = systable_beginscan(conrel, ConstraintParentIndexId,
+ true, NULL, 1, &pkey);
+
+ while (HeapTupleIsValid(childtup = systable_getnext(pscan)))
+ {
+ Oid childrelid;
+ Relation childrel;
+
+ childrelid = ((Form_pg_constraint) GETSTRUCT(childtup))->conrelid;
+ childrel = table_open(childrelid, lockmode);
+ ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, childrel, childtup,
+ otherrelids, lockmode);
+ table_close(childrel, NoLock);
+ }
+
+ systable_endscan(pscan);
+ }
+}
+
/*
* ALTER TABLE VALIDATE CONSTRAINT
*
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 7386f4d635..f87cd876d5 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2313,6 +2313,36 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
DELETE FROM pk WHERE a = 1;
DELETE FROM fk WHERE a = 1;
COMMIT; -- OK
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2));
+CREATE TABLE ref(f1 int, f2 int, f3 int)
+ PARTITION BY list(f1);
+CREATE TABLE ref1 PARTITION OF REF FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF REF FOR VALUES in (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+ DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+ PARTITION BY LIST(f1);
+CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2);
+CREATE TABLE ref(f1 int, f2 int, f3 int);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+ DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
DROP SCHEMA fkpart9 CASCADE;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to table pk
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 67aa20435d..da5f9fa951 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1636,6 +1636,39 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
DELETE FROM pk WHERE a = 1;
DELETE FROM fk WHERE a = 1;
COMMIT; -- OK
+
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2));
+CREATE TABLE ref(f1 int, f2 int, f3 int)
+ PARTITION BY list(f1);
+CREATE TABLE ref1 PARTITION OF REF FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF REF FOR VALUES in (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+ DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+ PARTITION BY LIST(f1);
+CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2);
+CREATE TABLE ref(f1 int, f2 int, f3 int);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+ DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
DROP SCHEMA fkpart9 CASCADE;
-- Verify ON UPDATE/DELETE behavior
view thread (8+ messages) latest in thread
Message-ID: <20210429230228.GA25949@alvherre.pgsql>
Permalink: ../20210429230228.GA25949@alvherre.pgsql/
Also on: postgresql.org/message-id/20210429230228.GA25949@alvherre.pgsql
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org
Subject: Re: ALTER CONSTRAINT on a partitioned FK isn't working
In-Reply-To: <20210429230228.GA25949@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox