Обсуждение: BUG #17817: DISABLE TRIGGER ALL on a partitioned table with foreign key fails

The following bug has been logged on the website:

Bug reference:      17817
Logged by:          Alan Hodgson
Email address:      ahodgson@simkin.ca
PostgreSQL version: 15.2
Operating system:   linux-amd64
Description:

This works on 14.7. It fails on 15.2. The Ruby on Rails test suites use
DISABLE TRIGGER ALL extensively.

BEGIN;
CREATE TABLE test_fk (id serial primary key);
CREATE TABLE test_table (test serial, created_at timestamp not null, fk_id
int not null references test_fk(id)) PARTITION BY RANGE (created_at);
CREATE TABLE test_table_2017 PARTITION OF test_table FOR VALUES FROM
('2017-01-01') TO ('2018-01-01');
ALTER TABLE test_table DISABLE TRIGGER ALL;
ROLLBACK;

ERROR:  trigger "RI_ConstraintTrigger_c_46838897" for table
"test_table_2017" does not exist

PG Bug reporting form <noreply@postgresql.org> writes:
> This works on 14.7. It fails on 15.2. The Ruby on Rails test suites use
> DISABLE TRIGGER ALL extensively.

> BEGIN;
> CREATE TABLE test_fk (id serial primary key);
> CREATE TABLE test_table (test serial, created_at timestamp not null, fk_id
> int not null references test_fk(id)) PARTITION BY RANGE (created_at);
> CREATE TABLE test_table_2017 PARTITION OF test_table FOR VALUES FROM
> ('2017-01-01') TO ('2018-01-01');
> ALTER TABLE test_table DISABLE TRIGGER ALL;
> ROLLBACK;
> ERROR:  trigger "RI_ConstraintTrigger_c_46838897" for table
> "test_table_2017" does not exist

Yeah, duplicated here.  Bisecting says it broke at

commit ec0925c22a3da7199650c9903a03a0017705ed5c
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date:   Thu Aug 4 20:02:02 2022 +0200

    Fix ENABLE/DISABLE TRIGGER to handle recursion correctly

    Using ATSimpleRecursion() in ATPrepCmd() to do so as bbb927b4db9b did is
    not correct, because ATPrepCmd() can't distinguish between triggers that
    may be cloned and those that may not, so would wrongly try to recurse
    for the latter category of triggers.

    So this commit restores the code in EnableDisableTrigger() that
    86f575948c77 had added to do the recursion, which would do it only for
    triggers that may be cloned, that is, row-level triggers.  This also
    changes tablecmds.c such that ATExecCmd() is able to pass the value of
    ONLY flag down to EnableDisableTrigger() using its new 'recurse'
    parameter.

Interestingly, although that commit was back-patched to v11, the failure
does not occur in pre-v15 branches.  So what's different about v15?

One clue is that the contents of pg_trigger are quite a bit different:

# select oid, tgparentid, tgrelid::regclass, tgname from pg_trigger where tgrelid in ('test_fk'::regclass,
'test_table'::regclass,'test_table_2017'::regclass); 
  oid  | tgparentid |     tgrelid     |            tgname
-------+------------+-----------------+------------------------------
 40997 |          0 | test_fk         | RI_ConstraintTrigger_a_40997
 40998 |          0 | test_fk         | RI_ConstraintTrigger_a_40998
 40999 |          0 | test_table      | RI_ConstraintTrigger_c_40999
 41000 |          0 | test_table      | RI_ConstraintTrigger_c_41000
 41006 |      40999 | test_table_2017 | RI_ConstraintTrigger_c_41006
 41007 |      41000 | test_table_2017 | RI_ConstraintTrigger_c_41007
(6 rows)

# ALTER TABLE test_table DISABLE TRIGGER ALL;
ERROR:  trigger "RI_ConstraintTrigger_c_40999" for table "test_table_2017" does not exist

whereas in v14 I see

# select oid, tgparentid, tgrelid::regclass, tgname from pg_trigger where tgrelid in ('test_fk'::regclass,
'test_table'::regclass,'test_table_2017'::regclass); 
  oid  | tgparentid |     tgrelid     |            tgname
-------+------------+-----------------+------------------------------
 38169 |          0 | test_fk         | RI_ConstraintTrigger_a_38169
 38170 |          0 | test_fk         | RI_ConstraintTrigger_a_38170
 38176 |          0 | test_table_2017 | RI_ConstraintTrigger_c_38176
 38177 |          0 | test_table_2017 | RI_ConstraintTrigger_c_38177
(4 rows)

It's a reasonable bet that we're trying to look up the child trigger
using the name of its parent trigger ... but why are we searching by
name at all, rather than OID?  Seems mighty failure-prone.

Stack trace from the lookup failure is

#0  errfinish (filename=0xac4aeb "trigger.c", lineno=1838,
    funcname=0xad8fe0 <__func__.30665> "EnableDisableTrigger") at elog.c:480
#1  0x00000000004c968f in EnableDisableTrigger (rel=<optimized out>,
    tgname=0x7f10283ad88c "RI_ConstraintTrigger_c_40999",
    fires_when=<optimized out>, skip_system=false, recurse=true, lockmode=6)
    at trigger.c:1835
#2  0x000000000069a717 in EnableDisableTrigger (rel=rel@entry=0x7f1031892768,
    tgname=tgname@entry=0x0, fires_when=fires_when@entry=68 'D',
    skip_system=false, recurse=true, lockmode=6) at trigger.c:1819
#3  0x0000000000691db4 in ATExecEnableDisableTrigger (
    lockmode=<optimized out>, recurse=<optimized out>,
    skip_system=<optimized out>, fires_when=<optimized out>,
    trigname=<optimized out>, rel=<optimized out>) at tablecmds.c:14729
#4  ATExecCmd (wqueue=0x7fffb8534408, tab=0x17f4788, cmd=<optimized out>,
    lockmode=6, cur_pass=<optimized out>, context=0x7fffb85345a0)
    at tablecmds.c:5165
#5  0x0000000000693108 in ATRewriteCatalogs (context=0x7fffb85345a0,
    lockmode=6, wqueue=0x7fffb8534408)
    at ../../../src/include/nodes/nodes.h:193

            regards, tom lane

On 2023-Mar-01, Tom Lane wrote:

> Interestingly, although that commit was back-patched to v11, the failure
> does not occur in pre-v15 branches.  So what's different about v15?

Hmm, I think f4566345cf40 probably explains that.

> It's a reasonable bet that we're trying to look up the child trigger
> using the name of its parent trigger ... but why are we searching by
> name at all, rather than OID?  Seems mighty failure-prone.

I have no recollection of this, but we probably didn't have the OID
originally.  It may be that simply changing that is enough to solve the
problem.  I'll try to have a look later today, but I'm not sure I'll
have time.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Mar-01, Tom Lane wrote:
>> It's a reasonable bet that we're trying to look up the child trigger
>> using the name of its parent trigger ... but why are we searching by
>> name at all, rather than OID?  Seems mighty failure-prone.

> I have no recollection of this, but we probably didn't have the OID
> originally.  It may be that simply changing that is enough to solve the
> problem.  I'll try to have a look later today, but I'm not sure I'll
> have time.

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

            regards, tom lane

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

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.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 char *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 *tgname,
     {
         Form_pg_trigger oldtrig = (Form_pg_trigger) GETSTRUCT(tuple);

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

                 part = 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 *trigname, 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 tgparent,
                                  char fires_when, bool skip_system, bool recurse,
                                  LOCKMODE lockmode);

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/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'::regclass)
+  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'::regclass)
+  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/triggers.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'::regclass)
+  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'::regclass)
+  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);

On 2023-Mar-03, Tom Lane wrote:

> 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.

Thank you, looks sane to me.

> 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.

Right ... triggers created in the normal way would have matching names,
so the previous code would work correctly.

I wonder how come this problem took so long to be detected with Ruby on
Rails; it's been in released 13.x and 14.x for seven months now.  I
suppose it would be very useful if the Ruby on Rails community would run
their tests more often on new Postgres versions (or even on the tip of
stable branches).

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Mar-03, Tom Lane wrote:
>> Basically just make the recursive steps match on tgparentid instead
>> of name, like this.

> Thank you, looks sane to me.

OK, will work on getting it committed.

> I wonder how come this problem took so long to be detected with Ruby on
> Rails; it's been in released 13.x and 14.x for seven months now.  I
> suppose it would be very useful if the Ruby on Rails community would run
> their tests more often on new Postgres versions (or even on the tip of
> stable branches).

Um ... 13.x and 14.x aren't showing the problem, or is there something
I missed?  But I agree that it'd be good if we could get some Ruby folk
to test more promptly --- it's pretty sad that this didn't get noticed
sooner in v15.

            regards, tom lane

I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> I wonder how come this problem took so long to be detected with Ruby on
>> Rails; it's been in released 13.x and 14.x for seven months now.

> Um ... 13.x and 14.x aren't showing the problem, or is there something
> I missed?

Oh!  Running the same test shows that while 12.x through 14.x do not
throw an error, they don't disable the child table's triggers either.

Moreover, we can't apply this fix idea since there is no tgparentid
linkage (or indeed any parent trigger to link to).

It's not hard to see one way to fix it: if the initial call is "for
all triggers", forget about recursing for individual triggers and
instead recursively do a "for all triggers" on the child.  However,
that would be the sort of semantics change that people tend to bitch
about in stable branches, because it'd nuke non-inherited triggers
too.

I'm kind of inclined to leave things alone pre-v15.  I asssume the
existing behavior had been that way all along, or do you have reason
to think it changed recently in those branches?

            regards, tom lane