Make ALTER TRIGGER RENAME consistent for partitioned tables

Renaming triggers on partitioned tables had two problems: first,
it did not recurse to renaming the triggers on the partitions; and
second, it failed to prohibit renaming clone triggers.  Having triggers
with different names in partitions is pointless, and furthermore pg_dump
would not preserve names for partitions anyway.

Not backpatched -- making the ALTER TRIGGER throw an error in stable
versions might cause problems for existing scripts.

Co-authored-by: Arne Roland <A.Roland@index.de>
Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Zhihong Yu <zyu@yugabyte.com>
Discussion: https://postgr.es/m/d0fd7040c2fb4de1a111b9d9ccc456b8@index.de

diff --git a/doc/src/sgml/ref/alter_trigger.sgml b/doc/src/sgml/ref/alter_trigger.sgml
index 43a7da4f0bcf39e150e0bde35227ba9972f3cb07..ece9cb5acfd1d90306c887eda421b16b5334e241 100644 (file)
--- a/doc/src/sgml/ref/alter_trigger.sgml
+++ b/doc/src/sgml/ref/alter_trigger.sgml
@@ -31,9 +31,20 @@ ALTER TRIGGER <replaceable class="parameter">name</replaceable> ON <replaceable
 
   <para>
    <command>ALTER TRIGGER</command> changes properties of an existing
-   trigger.  The <literal>RENAME</literal> clause changes the name of
+   trigger.
+  </para>
+
+  <para>
+   The <literal>RENAME</literal> clause changes the name of
    the given trigger without otherwise changing the trigger
-   definition.  The <literal>DEPENDS ON EXTENSION</literal> clause marks
+   definition.
+   If the table that the trigger is on is a partitioned table,
+   then corresponding clone triggers in the partitions are
+   renamed too.
+  </para>
+
+  <para>
+   The <literal>DEPENDS ON EXTENSION</literal> clause marks
    the trigger as dependent on an extension, such that if the extension is
    dropped, the trigger will automatically be dropped as well.
   </para>

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 6d4b7ee92acda30e8847ec7d71a6b1e2f924fa93..fc0a4b2fa738d9df80c3ee0229e0a570acc46d67 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -71,6 +71,12 @@ int          SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN;
 static int MyTriggerDepth = 0;
 
 /* Local function prototypes */
+static void renametrig_internal(Relation tgrel, Relation targetrel,
+                               HeapTuple trigtup, const char *newname,
+                               const char *expected_name);
+static void renametrig_partition(Relation tgrel, Oid partitionId,
+                                Oid parentTriggerOid, const char *newname,
+                                const char *expected_name);
 static void SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger);
 static bool GetTupleForTrigger(EState *estate,
                               EPQState *epqstate,
@@ -1442,38 +1448,16 @@ renametrig(RenameStmt *stmt)
    targetrel = relation_open(relid, NoLock);
 
    /*
-    * Scan pg_trigger twice for existing triggers on relation.  We do this in
-    * order to ensure a trigger does not exist with newname (The unique index
-    * on tgrelid/tgname would complain anyway) and to ensure a trigger does
-    * exist with oldname.
-    *
-    * NOTE that this is cool only because we have AccessExclusiveLock on the
-    * relation, so the trigger set won't be changing underneath us.
+    * On partitioned tables, this operation recurses to partitions.  Lock all
+    * tables upfront.
     */
-   tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+   if (targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+       (void) find_all_inheritors(relid, AccessExclusiveLock, NULL);
 
-   /*
-    * First pass -- look for name conflict
-    */
-   ScanKeyInit(&key[0],
-               Anum_pg_trigger_tgrelid,
-               BTEqualStrategyNumber, F_OIDEQ,
-               ObjectIdGetDatum(relid));
-   ScanKeyInit(&key[1],
-               Anum_pg_trigger_tgname,
-               BTEqualStrategyNumber, F_NAMEEQ,
-               PointerGetDatum(stmt->newname));
-   tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
-                               NULL, 2, key);
-   if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
-       ereport(ERROR,
-               (errcode(ERRCODE_DUPLICATE_OBJECT),
-                errmsg("trigger \"%s\" for relation \"%s\" already exists",
-                       stmt->newname, RelationGetRelationName(targetrel))));
-   systable_endscan(tgscan);
+   tgrel = table_open(TriggerRelationId, RowExclusiveLock);
 
    /*
-    * Second pass -- look for trigger existing with oldname and update
+    * Search for the trigger to modify.
     */
    ScanKeyInit(&key[0],
                Anum_pg_trigger_tgrelid,
@@ -1489,27 +1473,40 @@ renametrig(RenameStmt *stmt)
    {
        Form_pg_trigger trigform;
 
-       /*
-        * Update pg_trigger tuple with new tgname.
-        */
-       tuple = heap_copytuple(tuple);  /* need a modifiable copy */
        trigform = (Form_pg_trigger) GETSTRUCT(tuple);
        tgoid = trigform->oid;
 
-       namestrcpy(&trigform->tgname,
-                  stmt->newname);
+       /*
+        * If the trigger descends from a trigger on a parent partitioned
+        * table, reject the rename.  We don't allow a trigger in a partition
+        * to differ in name from that of its parent: that would lead to an
+        * inconsistency that pg_dump would not reproduce.
+        */
+       if (OidIsValid(trigform->tgparentid))
+           ereport(ERROR,
+                   errmsg("cannot rename trigger \"%s\" on table \"%s\"",
+                          stmt->subname, RelationGetRelationName(targetrel)),
+                   errhint("Rename trigger on partitioned table \"%s\" instead.",
+                           get_rel_name(get_partition_parent(relid, false))));
 
-       CatalogTupleUpdate(tgrel, &tuple->t_self, tuple);
 
-       InvokeObjectPostAlterHook(TriggerRelationId,
-                                 tgoid, 0);
+       /* Rename the trigger on this relation ... */
+       renametrig_internal(tgrel, targetrel, tuple, stmt->newname,
+                           stmt->subname);
 
-       /*
-        * Invalidate relation's relcache entry so that other backends (and
-        * this one too!) are sent SI message to make them rebuild relcache
-        * entries.  (Ideally this should happen automatically...)
-        */
-       CacheInvalidateRelcache(targetrel);
+       /* ... and if it is partitioned, recurse to its partitions */
+       if (targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+       {
+           PartitionDesc partdesc = RelationGetPartitionDesc(targetrel, true);
+
+           for (int i = 0; i < partdesc->nparts; i++)
+           {
+               Oid         partitionId = partdesc->oids[i];
+
+               renametrig_partition(tgrel, partitionId, trigform->oid,
+                                    stmt->newname, stmt->subname);
+           }
+       }
    }
    else
    {
@@ -1533,6 +1530,137 @@ renametrig(RenameStmt *stmt)
    return address;
 }
 
+/*
+ * Subroutine for renametrig -- perform the actual work of renaming one
+ * trigger on one table.
+ *
+ * If the trigger has a name different from the expected one, raise a
+ * NOTICE about it.
+ */
+static void
+renametrig_internal(Relation tgrel, Relation targetrel, HeapTuple trigtup,
+                   const char *newname, const char *expected_name)
+{
+   HeapTuple   tuple;
+   Form_pg_trigger tgform;
+   ScanKeyData key[2];
+   SysScanDesc tgscan;
+
+   /* If the trigger already has the new name, nothing to do. */
+   tgform = (Form_pg_trigger) GETSTRUCT(trigtup);
+   if (strcmp(NameStr(tgform->tgname), newname) == 0)
+       return;
+
+   /*
+    * Before actually trying the rename, search for triggers with the same
+    * name.  The update would fail with an ugly message in that case, and it
+    * is better to throw a nicer error.
+    */
+   ScanKeyInit(&key[0],
+               Anum_pg_trigger_tgrelid,
+               BTEqualStrategyNumber, F_OIDEQ,
+               ObjectIdGetDatum(RelationGetRelid(targetrel)));
+   ScanKeyInit(&key[1],
+               Anum_pg_trigger_tgname,
+               BTEqualStrategyNumber, F_NAMEEQ,
+               PointerGetDatum(newname));
+   tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
+                               NULL, 2, key);
+   if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
+       ereport(ERROR,
+               (errcode(ERRCODE_DUPLICATE_OBJECT),
+                errmsg("trigger \"%s\" for relation \"%s\" already exists",
+                       newname, RelationGetRelationName(targetrel))));
+   systable_endscan(tgscan);
+
+   /*
+    * The target name is free; update the existing pg_trigger tuple with it.
+    */
+   tuple = heap_copytuple(trigtup);    /* need a modifiable copy */
+   tgform = (Form_pg_trigger) GETSTRUCT(tuple);
+
+   /*
+    * If the trigger has a name different from what we expected, let the user
+    * know. (We can proceed anyway, since we must have reached here following
+    * a tgparentid link.)
+    */
+   if (strcmp(NameStr(tgform->tgname), expected_name) != 0)
+       ereport(NOTICE,
+               errmsg("renamed trigger \"%s\" on relation \"%s\"",
+                      NameStr(tgform->tgname),
+                      RelationGetRelationName(targetrel)));
+
+   namestrcpy(&tgform->tgname, newname);
+
+   CatalogTupleUpdate(tgrel, &tuple->t_self, tuple);
+
+   InvokeObjectPostAlterHook(TriggerRelationId, tgform->oid, 0);
+
+   /*
+    * Invalidate relation's relcache entry so that other backends (and this
+    * one too!) are sent SI message to make them rebuild relcache entries.
+    * (Ideally this should happen automatically...)
+    */
+   CacheInvalidateRelcache(targetrel);
+}
+
+/*
+ * Subroutine for renametrig -- Helper for recursing to partitions when
+ * renaming triggers on a partitioned table.
+ */
+static void
+renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
+                    const char *newname, const char *expected_name)
+{
+   SysScanDesc tgscan;
+   ScanKeyData key;
+   HeapTuple   tuple;
+   int         found PG_USED_FOR_ASSERTS_ONLY = 0;
+
+   /*
+    * Given a relation and the OID of a trigger on parent relation, find the
+    * corresponding trigger in the child and rename that trigger to the given
+    * name.
+    */
+   ScanKeyInit(&key,
+               Anum_pg_trigger_tgrelid,
+               BTEqualStrategyNumber, F_OIDEQ,
+               ObjectIdGetDatum(partitionId));
+   tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
+                               NULL, 1, &key);
+   while (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
+   {
+       Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tuple);
+       Relation    partitionRel;
+
+       if (tgform->tgparentid != parentTriggerOid)
+           continue;           /* not our trigger */
+
+       Assert(found++ <= 0);
+
+       partitionRel = table_open(partitionId, NoLock);
+
+       /* Rename the trigger on this partition */
+       renametrig_internal(tgrel, partitionRel, tuple, newname, expected_name);
+
+       /* And if this relation is partitioned, recurse to its partitions */
+       if (partitionRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+       {
+           PartitionDesc partdesc = RelationGetPartitionDesc(partitionRel,
+                                                             true);
+
+           for (int i = 0; i < partdesc->nparts; i++)
+           {
+               Oid         partitionId = partdesc->oids[i];
+
+               renametrig_partition(tgrel, partitionId, tgform->oid, newname,
+                                    NameStr(tgform->tgname));
+           }
+       }
+       table_close(partitionRel, NoLock);
+   }
+   systable_endscan(tgscan);
+}
 
 /*
  * EnableDisableTrigger()

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 5254447cf8ebefb1e0f54c210ff0568d7e8445df..564eb4faa24c3b45c2ebc41e4cdab67d4a7e8b8a 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3410,3 +3410,79 @@ for each statement execute function trigger_function1();
 delete from convslot_test_parent;
 NOTICE:  trigger = bdt_trigger, old_table = (111,tutu), (311,tutu)
 drop table convslot_test_child, convslot_test_parent;
+-- Test trigger renaming on partitioned tables
+create table grandparent (id int, primary key (id)) partition by range (id);
+create table middle partition of grandparent for values from (1) to (10)
+partition by range (id);
+create table chi partition of middle for values from (1) to (5);
+create table cho partition of middle for values from (6) to (10);
+create function f () returns trigger as
+$$ begin return new; end; $$
+language plpgsql;
+create trigger a after insert on grandparent
+for each row execute procedure f();
+alter trigger a on grandparent rename to b;
+select tgrelid::regclass, tgname,
+(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent'))
+order by tgname, tgrelid::regclass::text;
+   tgrelid   | tgname | parent_tgname 
+-------------+--------+---------------
+ chi         | b      | b
+ cho         | b      | b
+ grandparent | b      | 
+ middle      | b      | b
+(4 rows)
+
+alter trigger a on only grandparent rename to b;   -- ONLY not supported
+ERROR:  syntax error at or near "only"
+LINE 1: alter trigger a on only grandparent rename to b;
+                           ^
+alter trigger b on middle rename to c; -- can't rename trigger on partition
+ERROR:  cannot rename trigger "b" on table "middle"
+HINT:  Rename trigger on partitioned table "grandparent" instead.
+create trigger c after insert on middle
+for each row execute procedure f();
+alter trigger b on grandparent rename to c;
+ERROR:  trigger "c" for relation "middle" already exists
+-- Rename cascading does not affect statement triggers
+create trigger p after insert on grandparent for each statement execute function f();
+create trigger p after insert on middle for each statement execute function f();
+alter trigger p on grandparent rename to q;
+select tgrelid::regclass, tgname,
+(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent'))
+order by tgname, tgrelid::regclass::text;
+   tgrelid   | tgname | parent_tgname 
+-------------+--------+---------------
+ chi         | b      | b
+ cho         | b      | b
+ grandparent | b      | 
+ middle      | b      | b
+ chi         | c      | c
+ cho         | c      | c
+ middle      | c      | 
+ middle      | p      | 
+ grandparent | q      | 
+(9 rows)
+
+drop table grandparent;
+-- Trigger renaming does not recurse on legacy inheritance
+create table parent (a int);
+create table child () inherits (parent);
+create trigger parenttrig after insert on parent
+for each row execute procedure f();
+create trigger parenttrig after insert on child
+for each row execute procedure f();
+alter trigger parenttrig on parent rename to anothertrig;
+\d+ child
+                                   Table "public.child"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats target | Description 
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a      | integer |           |          |         | plain   |              | 
+Triggers:
+    parenttrig AFTER INSERT ON child FOR EACH ROW EXECUTE FUNCTION f()
+Inherits: parent
+
+drop table parent, child;
+drop function f();

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 7b73ee20a1b0de8ea614938d510a0c3b3d10921a..fb94eca3ed2ce1d8f5eb24fc8aeb92b7bbaf4c23 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2572,3 +2572,50 @@ for each statement execute function trigger_function1();
 delete from convslot_test_parent;
 
 drop table convslot_test_child, convslot_test_parent;
+
+-- Test trigger renaming on partitioned tables
+create table grandparent (id int, primary key (id)) partition by range (id);
+create table middle partition of grandparent for values from (1) to (10)
+partition by range (id);
+create table chi partition of middle for values from (1) to (5);
+create table cho partition of middle for values from (6) to (10);
+create function f () returns trigger as
+$$ begin return new; end; $$
+language plpgsql;
+create trigger a after insert on grandparent
+for each row execute procedure f();
+
+alter trigger a on grandparent rename to b;
+select tgrelid::regclass, tgname,
+(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent'))
+order by tgname, tgrelid::regclass::text;
+alter trigger a on only grandparent rename to b;   -- ONLY not supported
+alter trigger b on middle rename to c; -- can't rename trigger on partition
+create trigger c after insert on middle
+for each row execute procedure f();
+alter trigger b on grandparent rename to c;
+
+-- Rename cascading does not affect statement triggers
+create trigger p after insert on grandparent for each statement execute function f();
+create trigger p after insert on middle for each statement execute function f();
+alter trigger p on grandparent rename to q;
+select tgrelid::regclass, tgname,
+(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent'))
+order by tgname, tgrelid::regclass::text;
+
+drop table grandparent;
+
+-- Trigger renaming does not recurse on legacy inheritance
+create table parent (a int);
+create table child () inherits (parent);
+create trigger parenttrig after insert on parent
+for each row execute procedure f();
+create trigger parenttrig after insert on child
+for each row execute procedure f();
+alter trigger parenttrig on parent rename to anothertrig;
+\d+ child
+
+drop table parent, child;
+drop function f();