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.

This also fixes what seems like an oversight of 86f575948c77 that the
recursion to partition triggers would only occur if EnableDisableTrigger()
had actually changed the trigger.  It is more apt to recurse to inspect
partition triggers even if the parent's trigger didn't need to be
changed: only then can we be certain that all descendants share the same
state afterwards.

Backpatch all the way back to 11, like bbb927b4db9b.  Care is taken not
to break ABI compatibility (and that no catversion bump is needed.)

Co-authored-by: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Dmitry Koval <d.koval@postgrespro.ru>
Discussion: https://postgr.es/m/CA+HiwqG-cZT3XzGAnEgZQLoQbyfJApVwOTQaCaas1mhpf+4V5A@mail.gmail.com

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 88cfd55641dc527ab039acf5ee5ae5e722575a21..6c5a36400935384b4ce5890d2f2c35facff521e9 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -523,7 +523,8 @@ static void ATExecSetRelOptions(Relation rel, List *defList,
                                AlterTableType operation,
                                LOCKMODE lockmode);
 static void ATExecEnableDisableTrigger(Relation rel, const char *trigname,
-                                      char fires_when, bool skip_system, LOCKMODE lockmode);
+                                      char fires_when, bool skip_system, bool recurse,
+                                      LOCKMODE lockmode);
 static void ATExecEnableDisableRule(Relation rel, const char *rulename,
                                    char fires_when, LOCKMODE lockmode);
 static void ATPrepAddInherit(Relation child_rel);
@@ -3736,9 +3737,7 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
  * be done in this phase.  Generally, this phase acquires table locks,
  * checks permissions and relkind, and recurses to find child tables.
  *
- * ATRewriteCatalogs performs phase 2 for each affected table.  (Note that
- * phases 2 and 3 normally do no explicit recursion, since phase 1 already
- * did it --- although some subcommands have to recurse in phase 2 instead.)
+ * ATRewriteCatalogs performs phase 2 for each affected table.
  * Certain subcommands need to be performed before others to avoid
  * unnecessary conflicts; for example, DROP COLUMN should come before
  * ADD COLUMN.  Therefore phase 1 divides the subcommands into multiple
@@ -3746,6 +3745,12 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
  *
  * ATRewriteTables performs phase 3 for those tables that need it.
  *
+ * For most subcommand types, phases 2 and 3 do no explicit recursion,
+ * since phase 1 already does it.  However, for certain subcommand types
+ * it is only possible to determine how to recurse at phase 2 time; for
+ * those cases, phase 1 sets the cmd->recurse flag (or, in some older coding,
+ * changes the command subtype of a "Recurse" variant XXX to be cleaned up.)
+ *
  * Thanks to the magic of MVCC, an error anywhere along the way rolls back
  * the whole operation; we don't have to do anything special to clean up.
  *
@@ -4144,11 +4149,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
    tab = ATGetQueueEntry(wqueue, rel);
 
    /*
-    * Copy the original subcommand for each table.  This avoids conflicts
-    * when different child tables need to make different parse
-    * transformations (for example, the same column may have different column
-    * numbers in different children).  It also ensures that we don't corrupt
-    * the original parse tree, in case it is saved in plancache.
+    * Copy the original subcommand for each table, so we can scribble on it.
+    * This avoids conflicts when different child tables need to make
+    * different parse transformations (for example, the same column may have
+    * different column numbers in different children).  It also ensures that
+    * we don't corrupt the original parse tree, in case it is saved in
+    * plancache.
     */
    cmd = copyObject(cmd);
 
@@ -4411,8 +4417,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
        case AT_DisableTrigAll:
        case AT_DisableTrigUser:
            ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
-           if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-               ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
+           /* Set up recursion for phase 2; no other prep needed */
+           if (recurse)
+               cmd->recurse = true;
            pass = AT_PASS_MISC;
            break;
        case AT_EnableRule:     /* ENABLE/DISABLE RULE variants */
@@ -4731,35 +4738,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
            break;
        case AT_EnableTrig:     /* ENABLE TRIGGER name */
            ATExecEnableDisableTrigger(rel, cmd->name,
-                                      TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+                                      TRIGGER_FIRES_ON_ORIGIN, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_EnableAlwaysTrig:   /* ENABLE ALWAYS TRIGGER name */
            ATExecEnableDisableTrigger(rel, cmd->name,
-                                      TRIGGER_FIRES_ALWAYS, false, lockmode);
+                                      TRIGGER_FIRES_ALWAYS, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_EnableReplicaTrig:  /* ENABLE REPLICA TRIGGER name */
            ATExecEnableDisableTrigger(rel, cmd->name,
-                                      TRIGGER_FIRES_ON_REPLICA, false, lockmode);
+                                      TRIGGER_FIRES_ON_REPLICA, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_DisableTrig:    /* DISABLE TRIGGER name */
            ATExecEnableDisableTrigger(rel, cmd->name,
-                                      TRIGGER_DISABLED, false, lockmode);
+                                      TRIGGER_DISABLED, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_EnableTrigAll:  /* ENABLE TRIGGER ALL */
            ATExecEnableDisableTrigger(rel, NULL,
-                                      TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+                                      TRIGGER_FIRES_ON_ORIGIN, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
            ATExecEnableDisableTrigger(rel, NULL,
-                                      TRIGGER_DISABLED, false, lockmode);
+                                      TRIGGER_DISABLED, false,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
            ATExecEnableDisableTrigger(rel, NULL,
-                                      TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
+                                      TRIGGER_FIRES_ON_ORIGIN, true,
+                                      cmd->recurse,
+                                      lockmode);
            break;
        case AT_DisableTrigUser:    /* DISABLE TRIGGER USER */
            ATExecEnableDisableTrigger(rel, NULL,
-                                      TRIGGER_DISABLED, true, lockmode);
+                                      TRIGGER_DISABLED, true,
+                                      cmd->recurse,
+                                      lockmode);
            break;
 
        case AT_EnableRule:     /* ENABLE RULE name */
@@ -13801,9 +13824,11 @@ index_copy_data(Relation rel, RelFileNode newrnode)
  */
 static void
 ATExecEnableDisableTrigger(Relation rel, const char *trigname,
-                          char fires_when, bool skip_system, LOCKMODE lockmode)
+                          char fires_when, bool skip_system, bool recurse,
+                          LOCKMODE lockmode)
 {
-   EnableDisableTrigger(rel, trigname, fires_when, skip_system, lockmode);
+   EnableDisableTriggerNew(rel, trigname, fires_when, skip_system, recurse,
+                           lockmode);
 }
 
 /*

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 788b92c7b8a0feffbcec4d8563e5f7b46eaa731a..59b38d00ed4e81eb7097ab110df7f9558148a542 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1468,14 +1468,16 @@ renametrig(RenameStmt *stmt)
  *            enablement/disablement, this also defines when the trigger
  *            should be fired in session replication roles.
  * skip_system: if true, skip "system" triggers (constraint triggers)
+ * recurse: if true, recurse to partitions
  *
  * Caller should have checked permissions for the table; here we also
  * enforce that superuser privilege is required to alter the state of
  * system triggers
  */
 void
-EnableDisableTrigger(Relation rel, const char *tgname,
-                    char fires_when, bool skip_system, LOCKMODE lockmode)
+EnableDisableTriggerNew(Relation rel, const char *tgname,
+                       char fires_when, bool skip_system, bool recurse,
+                       LOCKMODE lockmode)
 {
    Relation    tgrel;
    int         nkeys;
@@ -1541,6 +1543,34 @@ EnableDisableTrigger(Relation rel, const char *tgname,
            changed = true;
        }
 
+       /*
+        * When altering FOR EACH ROW triggers on a partitioned table, do the
+        * same on the partitions as well, unless ONLY is specified.
+        *
+        * Note that we recurse even if we didn't change the trigger above,
+        * because the partitions' copy of the trigger may have a different
+        * value of tgenabled than the parent's trigger and thus might need to
+        * be changed.
+        */
+       if (recurse &&
+           rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+           (TRIGGER_FOR_ROW(oldtrig->tgtype)))
+       {
+           PartitionDesc partdesc = RelationGetPartitionDesc(rel);
+           int         i;
+
+           for (i = 0; i < partdesc->nparts; i++)
+           {
+               Relation    part;
+
+               part = relation_open(partdesc->oids[i], lockmode);
+               EnableDisableTriggerNew(part, NameStr(oldtrig->tgname),
+                                       fires_when, skip_system, recurse,
+                                       lockmode);
+               table_close(part, NoLock);  /* keep lock till commit */
+           }
+       }
+
        InvokeObjectPostAlterHook(TriggerRelationId,
                                  oldtrig->oid, 0);
    }
@@ -1564,6 +1594,19 @@ EnableDisableTrigger(Relation rel, const char *tgname,
        CacheInvalidateRelcache(rel);
 }
 
+/*
+ * ABI-compatible wrapper for the above.  To keep as close possible to the old
+ * behavior, this never recurses.  Do not call this function in new code.
+ */
+void
+EnableDisableTrigger(Relation rel, const char *tgname,
+                    char fires_when, bool skip_system,
+                    LOCKMODE lockmode)
+{
+   EnableDisableTriggerNew(rel, tgname, fires_when, skip_system,
+                           true, lockmode);
+}
+
 
 /*
  * Build trigger data to attach to the given relcache entry.

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 682b28ed729672efc5802a05f2112ec63078b262..8db09fda63c0ccccb0cd576ccb0b70ad4ee67dc4 100644 (file)
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3211,6 +3211,7 @@ _copyAlterTableCmd(const AlterTableCmd *from)
    COPY_NODE_FIELD(def);
    COPY_SCALAR_FIELD(behavior);
    COPY_SCALAR_FIELD(missing_ok);
+   COPY_SCALAR_FIELD(recurse);
 
    return newnode;
 }

diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 8dc50aea81144c02c69121f8f0c51d0856f70a99..092fb379237cbd436a552f3d768752d9b8a578eb 100644 (file)
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1103,6 +1103,7 @@ _equalAlterTableCmd(const AlterTableCmd *a, const AlterTableCmd *b)
    COMPARE_NODE_FIELD(def);
    COMPARE_SCALAR_FIELD(behavior);
    COMPARE_SCALAR_FIELD(missing_ok);
+   COMPARE_SCALAR_FIELD(recurse);
 
    return true;
 }

diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 40b8154876450dabb56f9d340d104ebbad045774..b76d91af05e5f6703a0686951591003c7dde5953 100644 (file)
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -173,6 +173,9 @@ extern Oid  get_trigger_oid(Oid relid, const char *name, bool missing_ok);
 
 extern ObjectAddress renametrig(RenameStmt *stmt);
 
+extern void EnableDisableTriggerNew(Relation rel, const char *tgname,
+                                   char fires_when, bool skip_system, bool recurse,
+                                   LOCKMODE lockmode);
 extern void EnableDisableTrigger(Relation rel, const char *tgname,
                                 char fires_when, bool skip_system, LOCKMODE lockmode);
 

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b907e97207212b408131a8f5fd6c563dc238cc48..943848782f40e853e35bda5809d1357d2d75ebd6 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1876,6 +1876,7 @@ typedef struct AlterTableCmd  /* one subcommand of an ALTER TABLE */
                                 * constraint, or parent table */
    DropBehavior behavior;      /* RESTRICT or CASCADE for DROP cases */
    bool        missing_ok;     /* skip error if missing? */
+   bool        recurse;        /* exec-time recursion */
 } AlterTableCmd;
 
 

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index fd383503407c537729f45428977fc3b0b4490a1d..8c3bee2bf1774ae5613bfbdcc6021a52f45e8b1b 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2655,24 +2655,42 @@ create table parent (a int) partition by list (a);
 create table child1 partition of parent for values in (1);
 create trigger tg after insert on parent
   for each row execute procedure trig_nothing();
+create trigger tg_stmt after insert on parent
+  for statement execute procedure trig_nothing();
 select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;
- tgrelid | tgname | tgenabled 
----------+--------+-----------
- child1  | tg     | O
- parent  | tg     | O
-(2 rows)
+ tgrelid | tgname  | tgenabled 
+---------+---------+-----------
+ child1  | tg      | O
+ parent  | tg      | O
+ parent  | tg_stmt | O
+(3 rows)
 
-alter table only parent enable always trigger tg;
+alter table only parent enable always trigger tg;  -- no recursion because ONLY
+alter table parent enable always trigger tg_stmt;  -- no recursion because statement trigger
 select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;
- tgrelid | tgname | tgenabled 
----------+--------+-----------
- child1  | tg     | O
- parent  | tg     | A
-(2 rows)
+ tgrelid | tgname  | tgenabled 
+---------+---------+-----------
+ child1  | tg      | O
+ parent  | tg      | A
+ parent  | tg_stmt | A
+(3 rows)
+
+-- The following is a no-op for the parent trigger but not so
+-- for the child trigger, so recursion should be applied.
+alter table parent enable always trigger tg;
+select tgrelid::regclass, tgname, tgenabled from pg_trigger
+  where tgrelid in ('parent'::regclass, 'child1'::regclass)
+  order by tgrelid::regclass::text;
+ tgrelid | tgname  | tgenabled 
+---------+---------+-----------
+ child1  | tg      | A
+ parent  | tg      | A
+ parent  | tg_stmt | A
+(3 rows)
 
 drop table parent, child1;
 -- Verify that firing state propagates correctly on creation, too

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 353dd6a307a76e2946f423d033a4d3e0acb21ba4..b9c49c2cb7868b633177eba39ea0fbf353b61f8f 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1832,10 +1832,19 @@ create table parent (a int) partition by list (a);
 create table child1 partition of parent for values in (1);
 create trigger tg after insert on parent
   for each row execute procedure trig_nothing();
+create trigger tg_stmt after insert on parent
+  for statement execute procedure trig_nothing();
 select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;
-alter table only parent enable always trigger tg;
+alter table only parent enable always trigger tg;  -- no recursion because ONLY
+alter table parent enable always trigger tg_stmt;  -- no recursion because statement trigger
+select tgrelid::regclass, tgname, tgenabled from pg_trigger
+  where tgrelid in ('parent'::regclass, 'child1'::regclass)
+  order by tgrelid::regclass::text;
+-- The following is a no-op for the parent trigger but not so
+-- for the child trigger, so recursion should be applied.
+alter table parent enable always trigger tg;
 select tgrelid::regclass, tgname, tgenabled from pg_trigger
   where tgrelid in ('parent'::regclass, 'child1'::regclass)
   order by tgrelid::regclass::text;