From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: ahodgson@simkin.ca, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17817: DISABLE TRIGGER ALL on a partitioned table with
 foreign key fails
In-reply-to: <2416983.1677854831@sss.pgh.pa.us>
References: <20230303112144.tzw3yeos2c5w3q6y@alvherre.pgsql>
 <2416983.1677854831@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 03 Mar 2023 09:47:11 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2473113.1677858599.0@sss.pgh.pa.us>
Date: Fri, 03 Mar 2023 10:50:40 -0500
Message-ID: <2473213.1677858640@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2473213.1677858640%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2473113.1677858599.1@sss.pgh.pa.us>

I wrote:
> I can throw together a patch for what I was thinking of.

Basically just make the recursive steps match on tgparentid instead
of name, like this.

I wonder whether anyplace else is making a similar mistake?  Although
there's not much we will let you do to a foreign key trigger, so it
might not matter for anything else.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bug-17817.patch"; charset="us-ascii"
Content-ID: <2473113.1677858599.2@sss.pgh.pa.us>
Content-Description: fix-bug-17817.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/table=
cmds.c
index 62d9917ca3..3a93c41d6a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14726,7 +14726,8 @@ ATExecEnableDisableTrigger(Relation rel, const cha=
r *trigname,
 						   char fires_when, bool skip_system, bool recurse,
 						   LOCKMODE lockmode)
 {
-	EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
+	EnableDisableTrigger(rel, trigname, InvalidOid,
+						 fires_when, skip_system, recurse,
 						 lockmode);
 }
 =

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger=
.c
index d7abde170b..66401f2839 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1715,7 +1715,8 @@ renametrig_partition(Relation tgrel, Oid partitionId=
, Oid parentTriggerOid,
  *	to change 'tgenabled' field for the specified trigger(s)
  *
  * rel: relation to process (caller must hold suitable lock on it)
- * tgname: trigger to process, or NULL to scan all triggers
+ * tgname: name of trigger to process, or NULL to scan all triggers
+ * tgparent: if not zero, process only triggers with this tgparentid
  * fires_when: new value for tgenabled field. In addition to generic
  *			   enablement/disablement, this also defines when the trigger
  *			   should be fired in session replication roles.
@@ -1727,7 +1728,7 @@ renametrig_partition(Relation tgrel, Oid partitionId=
, Oid parentTriggerOid,
  * system triggers
  */
 void
-EnableDisableTrigger(Relation rel, const char *tgname,
+EnableDisableTrigger(Relation rel, const char *tgname, Oid tgparent,
 					 char fires_when, bool skip_system, bool recurse,
 					 LOCKMODE lockmode)
 {
@@ -1766,6 +1767,9 @@ EnableDisableTrigger(Relation rel, const char *tgnam=
e,
 	{
 		Form_pg_trigger oldtrig =3D (Form_pg_trigger) GETSTRUCT(tuple);
 =

+		if (OidIsValid(tgparent) && tgparent !=3D oldtrig->tgparentid)
+			continue;
+
 		if (oldtrig->tgisinternal)
 		{
 			/* system trigger ... ok to process? */
@@ -1816,7 +1820,8 @@ EnableDisableTrigger(Relation rel, const char *tgnam=
e,
 				Relation	part;
 =

 				part =3D relation_open(partdesc->oids[i], lockmode);
-				EnableDisableTrigger(part, NameStr(oldtrig->tgname),
+				/* Match on child triggers' tgparentid, not their name */
+				EnableDisableTrigger(part, NULL, oldtrig->oid,
 									 fires_when, skip_system, recurse,
 									 lockmode);
 				table_close(part, NoLock);	/* keep lock till commit */
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger=
.h
index 39d764dac7..67496d84f9 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -170,7 +170,7 @@ extern Oid	get_trigger_oid(Oid relid, const char *trig=
name, bool missing_ok);
 =

 extern ObjectAddress renametrig(RenameStmt *stmt);
 =

-extern void EnableDisableTrigger(Relation rel, const char *tgname,
+extern void EnableDisableTrigger(Relation rel, const char *tgname, Oid tg=
parent,
 								 char fires_when, bool skip_system, bool recurse,
 								 LOCKMODE lockmode);
 =

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/exp=
ected/triggers.out
index 7dbeced570..f7301b5213 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2718,6 +2718,40 @@ select tgrelid::regclass, tgname, tgenabled from pg=
_trigger
  parent  | tg_stmt | A
 (3 rows)
 =

+drop table parent, child1;
+-- Check processing of foreign key triggers
+create table parent (a int primary key, f int references parent)
+  partition by list (a);
+create table child1 partition of parent for values in (1);
+select tgrelid::regclass, rtrim(tgname, '0123456789') as tgname,
+  tgfoid::regproc, tgenabled
+  from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclas=
s)
+  order by tgrelid::regclass::text, tgfoid;
+ tgrelid |         tgname          |         tgfoid         | tgenabled =

+---------+-------------------------+------------------------+-----------
+ child1  | RI_ConstraintTrigger_c_ | "RI_FKey_check_ins"    | O
+ child1  | RI_ConstraintTrigger_c_ | "RI_FKey_check_upd"    | O
+ parent  | RI_ConstraintTrigger_c_ | "RI_FKey_check_ins"    | O
+ parent  | RI_ConstraintTrigger_c_ | "RI_FKey_check_upd"    | O
+ parent  | RI_ConstraintTrigger_a_ | "RI_FKey_noaction_del" | O
+ parent  | RI_ConstraintTrigger_a_ | "RI_FKey_noaction_upd" | O
+(6 rows)
+
+alter table parent disable trigger all;
+select tgrelid::regclass, rtrim(tgname, '0123456789') as tgname,
+  tgfoid::regproc, tgenabled
+  from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclas=
s)
+  order by tgrelid::regclass::text, tgfoid;
+ tgrelid |         tgname          |         tgfoid         | tgenabled =

+---------+-------------------------+------------------------+-----------
+ child1  | RI_ConstraintTrigger_c_ | "RI_FKey_check_ins"    | D
+ child1  | RI_ConstraintTrigger_c_ | "RI_FKey_check_upd"    | D
+ parent  | RI_ConstraintTrigger_c_ | "RI_FKey_check_ins"    | D
+ parent  | RI_ConstraintTrigger_c_ | "RI_FKey_check_upd"    | D
+ parent  | RI_ConstraintTrigger_a_ | "RI_FKey_noaction_del" | D
+ parent  | RI_ConstraintTrigger_a_ | "RI_FKey_noaction_upd" | D
+(6 rows)
+
 drop table parent, child1;
 -- Verify that firing state propagates correctly on creation, too
 CREATE TABLE trgfire (i int) PARTITION BY RANGE (i);
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/trig=
gers.sql
index 4d8504fb24..4222a8500b 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1883,6 +1883,21 @@ select tgrelid::regclass, tgname, tgenabled from pg=
_trigger
   order by tgrelid::regclass::text, tgname;
 drop table parent, child1;
 =

+-- Check processing of foreign key triggers
+create table parent (a int primary key, f int references parent)
+  partition by list (a);
+create table child1 partition of parent for values in (1);
+select tgrelid::regclass, rtrim(tgname, '0123456789') as tgname,
+  tgfoid::regproc, tgenabled
+  from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclas=
s)
+  order by tgrelid::regclass::text, tgfoid;
+alter table parent disable trigger all;
+select tgrelid::regclass, rtrim(tgname, '0123456789') as tgname,
+  tgfoid::regproc, tgenabled
+  from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclas=
s)
+  order by tgrelid::regclass::text, tgfoid;
+drop table parent, child1;
+
 -- Verify that firing state propagates correctly on creation, too
 CREATE TABLE trgfire (i int) PARTITION BY RANGE (i);
 CREATE TABLE trgfire1 PARTITION OF trgfire FOR VALUES FROM (1) TO (10);

------- =_aaaaaaaaaa0--