From 6949b921d545809a83f8a6bad4948f9012a76fb6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 4 Mar 2023 13:32:35 -0500 Subject: [PATCH] Avoid failure when altering state of partitioned foreign-key triggers. Beginning in v15, if you apply ALTER TABLE ENABLE/DISABLE TRIGGER to a partitioned table, it also affects the partitions' cloned versions of the affected trigger(s). The initial implementation of this located the clones by name, but that fails on foreign-key triggers which have names incorporating their own OIDs. We can fix that, and also make the behavior more bulletproof in the face of user-initiated trigger renames, by identifying the cloned triggers by tgparentid. Following the lead of earlier commits in this area, I took care not to break ABI in the v15 branch, even though I rather doubt there are any external callers of EnableDisableTrigger. While here, update the documentation, which was not touched when the semantics were changed. Per bug #17817 from Alan Hodgson. Back-patch to v15; older versions do not have this behavior. Discussion: https://postgr.es/m/17817-31dfb7c2100d9f3d@postgresql.org --- doc/src/sgml/ref/alter_table.sgml | 23 +++++++++++------ src/backend/commands/tablecmds.c | 3 ++- src/backend/commands/trigger.c | 11 ++++++--- src/include/commands/trigger.h | 2 +- src/test/regress/expected/triggers.out | 34 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 15 ++++++++++++ 6 files changed, 76 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 9aaa32a782..d4d93eeb7c 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -576,12 +576,12 @@ WITH ( MODULUS numeric_literal, REM These forms configure the firing of trigger(s) belonging to the table. A disabled trigger is still known to the system, but is not executed - when its triggering event occurs. For a deferred trigger, the enable + when its triggering event occurs. (For a deferred trigger, the enable status is checked when the event occurs, not when the trigger function - is actually executed. One can disable or enable a single + is actually executed.) One can disable or enable a single trigger specified by name, or all triggers on the table, or only user triggers (this option excludes internally generated constraint - triggers such as those that are used to implement foreign key + triggers, such as those that are used to implement foreign key constraints or deferrable uniqueness and exclusion constraints). Disabling or enabling internally generated constraint triggers requires superuser privileges; it should be done with caution since @@ -603,7 +603,7 @@ WITH ( MODULUS numeric_literal, REM The effect of this mechanism is that in the default configuration, triggers do not fire on replicas. This is useful because if a trigger is used on the origin to propagate data between tables, then the - replication system will also replicate the propagated data, and the + replication system will also replicate the propagated data; so the trigger should not fire a second time on the replica, because that would lead to duplication. However, if a trigger is used for another purpose such as creating external alerts, then it might be appropriate to set it @@ -611,6 +611,12 @@ WITH ( MODULUS numeric_literal, REM replicas. + + When this command is applied to a partitioned table, the states of + corresponding clone triggers in the partitions are updated too, + unless ONLY is specified. + + This command acquires a SHARE ROW EXCLUSIVE lock. @@ -1239,7 +1245,7 @@ WITH ( MODULUS numeric_literal, REM Disable or enable all triggers belonging to the table. (This requires superuser privilege if any of the triggers are - internally generated constraint triggers such as those that are used + internally generated constraint triggers, such as those that are used to implement foreign key constraints or deferrable uniqueness and exclusion constraints.) @@ -1251,7 +1257,7 @@ WITH ( MODULUS numeric_literal, REM Disable or enable all triggers belonging to the table except for - internally generated constraint triggers such as those that are used + internally generated constraint triggers, such as those that are used to implement foreign key constraints or deferrable uniqueness and exclusion constraints. @@ -1504,9 +1510,12 @@ WITH ( MODULUS numeric_literal, REM The actions for identity columns (ADD GENERATED, SET etc., DROP IDENTITY), as well as the actions - TRIGGER, CLUSTER, OWNER, + CLUSTER, OWNER, and TABLESPACE never recurse to descendant tables; that is, they always act as though ONLY were specified. + Actions affecting trigger states recurse to partitions of partitioned + tables (unless ONLY is specified), but never to + traditional-inheritance descendants. Adding a constraint recurses only for CHECK constraints that are not marked NO INHERIT. 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); -- 2.39.5