enable/disable broken for statement triggers on partitioned tables
Hi,
Simon reported $subject off-list.
For triggers on partitioned tables, various enable/disable trigger
variants recurse to also process partitions' triggers by way of
ATSimpleRecursion() done in the "prep" phase. While that way of
recursing is fine for row-level triggers which are cloned to
partitions, it isn't for statement-level triggers which are not
cloned, so you get an unexpected error as follows:
create table p (a int primary key) partition by list (a);
create table p1 partition of p for values in (1);
create function trigfun () returns trigger language plpgsql as $$
begin raise notice 'insert on p'; end; $$;
create trigger trig before insert on p for statement execute function trigfun();
alter table p disable trigger trig;
ERROR: trigger "trig" for table "p1" does not exist
The problem is that ATPrepCmd() is too soon to perform the recursion
in this case as it's not known at that stage if the trigger being
enabled/disabled is row-level or statement level, so it's better to
perform it during ATExecCmd(). Actually, that is how it used to be
done before bbb927b4db9b changed things to use ATSimpleRecursion() to
fix a related problem, which was that the ONLY specification was
ignored by the earlier implementation. The information of whether
ONLY is specified in a given command is only directly available in the
"prep" phase and must be remembered somehow if the recursion must be
handled in the "exec" phase. The way that's typically done that I see
in tablecmds.c is to have ATPrepCmd() change the AlterTableCmd.subtype
to a recursive variant of a given sub-command. For example,
AT_ValidateConstraint by AT_ValidateConstraintRecurse if ONLY is not
specified.
So, I think we should do something like the attached. A lot of
boilerplate is needed given that the various enable/disable trigger
variants are represented as separate sub-commands (AlterTableCmd
subtypes), which can perhaps be avoided by inventing a
EnableDisableTrigStmt sub-command node that stores (only?) the recurse
flag.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patchapplication/octet-stream; name=v1-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patchDownload
From 01a71dc0af9f2d6190863754aabbe1bc04bc40e4 Mon Sep 17 00:00:00 2001
From: amitlan <amitlangote09@gmail.com>
Date: Tue, 24 May 2022 12:43:48 +0900
Subject: [PATCH v1] Fix ENABLE/DISABLE TRIGGER to handle recursion correctly
Using ATSimpleRecursion() as bbb927b4 taught it to do is not correct,
because it can't understand that some triggers are not cloned, so need
not be recursivel handled.
This restores the original code in trigger.c: EnableDisableTrigger()
to do the recursion so that it is only performed for triggers that
are cloned -- only row-level triggers -- and also changes tablecmds.c
to pass the value of the ONLY flag down to EnableDisableTrigger() so
the latter knows whether or not the ALTER TABLE command has requested
recursion.
---
src/backend/commands/tablecmds.c | 131 ++++++++++++++++++++++---
src/backend/commands/trigger.c | 27 ++++-
src/include/commands/trigger.h | 3 +-
src/include/nodes/parsenodes.h | 8 ++
src/test/regress/expected/triggers.out | 27 ++---
src/test/regress/sql/triggers.sql | 5 +-
6 files changed, 174 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2de0ebacec..a09680da9b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -579,7 +579,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);
@@ -4756,16 +4757,59 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* No command-specific prep needed */
break;
case AT_EnableTrig: /* ENABLE TRIGGER variants */
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableAlwaysTrig:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableAlwaysTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableReplicaTrig:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableReplicaTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableTrigAll:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigAllRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableTrigUser:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigUserRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrig: /* DISABLE TRIGGER variants */
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrigAll:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigAllRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrigUser:
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigUserRecurse;
pass = AT_PASS_MISC;
break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -5106,35 +5150,86 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_EnableTrig: /* ENABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false, false,
+ lockmode);
+ break;
+ case AT_EnableTrigRecurse: /* ENABLE TRIGGER name with recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ON_ORIGIN, false, true,
+ lockmode);
break;
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ALWAYS, false, lockmode);
+ TRIGGER_FIRES_ALWAYS, false, false,
+ lockmode);
+ break;
+ case AT_EnableAlwaysTrigRecurse: /* ENABLE ALWAYS TRIGGER name with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ALWAYS, false, true,
+ lockmode);
break;
case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ON_REPLICA, false, lockmode);
+ TRIGGER_FIRES_ON_REPLICA, false, false,
+ lockmode);
+ break;
+ case AT_EnableReplicaTrigRecurse: /* ENABLE REPLICA TRIGGER name with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ON_REPLICA, false, true,
+ lockmode);
break;
case AT_DisableTrig: /* DISABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false, false,
+ lockmode);
+ break;
+ case AT_DisableTrigRecurse: /* DISABLE TRIGGER name with recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_DISABLED, false, true,
+ lockmode);
break;
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false, false,
+ lockmode);
+ break;
+ case AT_EnableTrigAllRecurse: /* ENABLE TRIGGER ALL with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_FIRES_ON_ORIGIN, false, true,
+ lockmode);
break;
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false, false,
+ lockmode);
+ break;
+ case AT_DisableTrigAllRecurse: /* DISABLE TRIGGER ALL with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_DISABLED, false, true,
+ lockmode);
break;
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, true, false,
+ lockmode);
+ break;
+ case AT_EnableTrigUserRecurse: /* ENABLE TRIGGER USER with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_FIRES_ON_ORIGIN, true, true,
+ lockmode);
break;
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, true, lockmode);
+ TRIGGER_DISABLED, true, false,
+ lockmode);
+ break;
+ case AT_DisableTrigUserRecurse: /* DISABLE TRIGGER USER with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_DISABLED, true, true,
+ lockmode);
break;
case AT_EnableRule: /* ENABLE RULE name */
@@ -6146,20 +6241,28 @@ alter_table_type_to_string(AlterTableType cmdtype)
case AT_ReplaceRelOptions:
return NULL; /* not real grammar */
case AT_EnableTrig:
+ case AT_EnableTrigRecurse:
return "ENABLE TRIGGER";
case AT_EnableAlwaysTrig:
+ case AT_EnableAlwaysTrigRecurse:
return "ENABLE ALWAYS TRIGGER";
case AT_EnableReplicaTrig:
+ case AT_EnableReplicaTrigRecurse:
return "ENABLE REPLICA TRIGGER";
case AT_DisableTrig:
+ case AT_DisableTrigRecurse:
return "DISABLE TRIGGER";
case AT_EnableTrigAll:
+ case AT_EnableTrigAllRecurse:
return "ENABLE TRIGGER ALL";
case AT_DisableTrigAll:
+ case AT_DisableTrigAllRecurse:
return "DISABLE TRIGGER ALL";
case AT_EnableTrigUser:
+ case AT_EnableTrigUserRecurse:
return "ENABLE TRIGGER USER";
case AT_DisableTrigUser:
+ case AT_DisableTrigUserRecurse:
return "DISABLE TRIGGER USER";
case AT_EnableRule:
return "ENABLE RULE";
@@ -14690,9 +14793,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);
+ EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
+ lockmode);
}
/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 13cb516752..94aec8d7d1 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1759,6 +1759,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
* 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
@@ -1766,7 +1767,8 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
*/
void
EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode)
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode)
{
Relation tgrel;
int nkeys;
@@ -1829,6 +1831,29 @@ EnableDisableTrigger(Relation rel, const char *tgname,
heap_freetuple(newtup);
+ /*
+ * When altering FOR EACH ROW triggers on a partitioned table, do
+ * the same on the partitions as well, unless ONLY is specified.
+ */
+ if (recurse &&
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+ (TRIGGER_FOR_ROW(oldtrig->tgtype)))
+ {
+ PartitionDesc partdesc = RelationGetPartitionDesc(rel, true);
+ int i;
+
+ for (i = 0; i < partdesc->nparts; i++)
+ {
+ Relation part;
+
+ part = relation_open(partdesc->oids[i], lockmode);
+ EnableDisableTrigger(part, NameStr(oldtrig->tgname),
+ fires_when, skip_system, recurse,
+ lockmode);
+ table_close(part, NoLock); /* keep lock till commit */
+ }
+ }
+
changed = true;
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 194e8d5bc1..b7b6bd6008 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
extern ObjectAddress renametrig(RenameStmt *stmt);
extern void EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode);
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode);
extern void RelationBuildTriggers(Relation relation);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 73f635b455..d9644c72eb 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2267,13 +2267,21 @@ typedef enum AlterTableType
AT_ResetRelOptions, /* RESET (...) -- AM specific parameters */
AT_ReplaceRelOptions, /* replace reloption list in its entirety */
AT_EnableTrig, /* ENABLE TRIGGER name */
+ AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */
+ AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableReplicaTrig, /* ENABLE REPLICA TRIGGER name */
+ AT_EnableReplicaTrigRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrig, /* DISABLE TRIGGER name */
+ AT_DisableTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableTrigAll, /* ENABLE TRIGGER ALL */
+ AT_EnableTrigAllRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrigAll, /* DISABLE TRIGGER ALL */
+ AT_DisableTrigAllRecurse, /* internal to commands/tablecmds.c */
AT_EnableTrigUser, /* ENABLE TRIGGER USER */
+ AT_EnableTrigUserRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrigUser, /* DISABLE TRIGGER USER */
+ AT_DisableTrigUserRecurse, /* internal to commands/tablecmds.c */
AT_EnableRule, /* ENABLE RULE name */
AT_EnableAlwaysRule, /* ENABLE ALWAYS RULE name */
AT_EnableReplicaRule, /* ENABLE REPLICA RULE name */
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index cd812336f2..05bc71ee7e 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2681,24 +2681,29 @@ 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)
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 83cd00f54f..5cf1e90e84 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1865,10 +1865,13 @@ 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;
--
2.35.3
Hi,
AT_EnableTrig, /* ENABLE TRIGGER name */
+ AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */
+ AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */Is it better to put the new enum's at the end of the AlterTableType?
This way the numeric values for existing ones don't change.
Cheers
Import Notes
Resolved by subject fallback
On 24.05.22 23:23, Zhihong Yu wrote:
Hi,
AT_EnableTrig, /* ENABLE TRIGGER name */ + AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */ AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */ + AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */Is it better to put the new enum's at the end of the AlterTableType?
This way the numeric values for existing ones don't change.
That's a concern if backpatching. Otherwise, it's better to put them
like shown in the patch.
On Fri, May 27, 2022 at 5:11 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
On 24.05.22 23:23, Zhihong Yu wrote:
Hi,
AT_EnableTrig, /* ENABLE TRIGGER name */ + AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */ AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */ + AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */Is it better to put the new enum's at the end of the AlterTableType?
This way the numeric values for existing ones don't change.
That's a concern if backpatching. Otherwise, it's better to put them
like shown in the patch.
Agreed.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On Tue, May 24, 2022 at 3:11 PM Amit Langote <amitlangote09@gmail.com> wrote:
Simon reported $subject off-list.
For triggers on partitioned tables, various enable/disable trigger
variants recurse to also process partitions' triggers by way of
ATSimpleRecursion() done in the "prep" phase. While that way of
recursing is fine for row-level triggers which are cloned to
partitions, it isn't for statement-level triggers which are not
cloned, so you get an unexpected error as follows:create table p (a int primary key) partition by list (a);
create table p1 partition of p for values in (1);
create function trigfun () returns trigger language plpgsql as $$
begin raise notice 'insert on p'; end; $$;
create trigger trig before insert on p for statement execute function trigfun();
alter table p disable trigger trig;
ERROR: trigger "trig" for table "p1" does not existThe problem is that ATPrepCmd() is too soon to perform the recursion
in this case as it's not known at that stage if the trigger being
enabled/disabled is row-level or statement level, so it's better to
perform it during ATExecCmd(). Actually, that is how it used to be
done before bbb927b4db9b changed things to use ATSimpleRecursion() to
fix a related problem, which was that the ONLY specification was
ignored by the earlier implementation. The information of whether
ONLY is specified in a given command is only directly available in the
"prep" phase and must be remembered somehow if the recursion must be
handled in the "exec" phase. The way that's typically done that I see
in tablecmds.c is to have ATPrepCmd() change the AlterTableCmd.subtype
to a recursive variant of a given sub-command. For example,
AT_ValidateConstraint by AT_ValidateConstraintRecurse if ONLY is not
specified.So, I think we should do something like the attached. A lot of
boilerplate is needed given that the various enable/disable trigger
variants are represented as separate sub-commands (AlterTableCmd
subtypes), which can perhaps be avoided by inventing a
EnableDisableTrigStmt sub-command node that stores (only?) the recurse
flag.
Added to the next CF: https://commitfest.postgresql.org/38/3728/
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Hi!
I've looked through the code and everything looks good.
But there is one thing I doubt.
Patch changes result of test:
----
create function trig_nothing() returns trigger language plpgsql
as $$ begin return null; end $$;
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();
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; -- no recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
where tgrelid in ('parent'::regclass, 'child1'::regclass)
order by tgrelid::regclass::text;
alter table parent enable always trigger tg; -- recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
where tgrelid in ('parent'::regclass, 'child1'::regclass)
order by tgrelid::regclass::text;
drop table parent, child1;
drop function trig_nothing();
----
Results of vanilla + patch:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | O
(2 rows)
ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)
ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)
DROP TABLE
DROP FUNCTION
----
Results of vanilla:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | O
(2 rows)
ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)
ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | A
parent | tg | A
(2 rows)
DROP TABLE
DROP FUNCTION
----
The patch doesn't start recursion in case 'tgenabled' flag of parent
table is not changes.
Probably vanilla result is more correct.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.com
Hi,
On Fri, Jul 8, 2022 at 3:44 AM Dmitry Koval <d.koval@postgrespro.ru> wrote:
I've looked through the code and everything looks good.
But there is one thing I doubt.
Patch changes result of test:
----create function trig_nothing() returns trigger language plpgsql
as $$ begin return null; end $$;
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();
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; -- no recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
where tgrelid in ('parent'::regclass, 'child1'::regclass)
order by tgrelid::regclass::text;
alter table parent enable always trigger tg; -- recursion
select tgrelid::regclass, tgname, tgenabled from pg_trigger
where tgrelid in ('parent'::regclass, 'child1'::regclass)
order by tgrelid::regclass::text;drop table parent, child1;
drop function trig_nothing();----
Results of vanilla + patch:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | O
(2 rows)ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)DROP TABLE
DROP FUNCTION----
Results of vanilla:
----
CREATE FUNCTION
CREATE TABLE
CREATE TABLE
CREATE TRIGGER
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | O
(2 rows)ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | O
parent | tg | A
(2 rows)ALTER TABLE
tgrelid | tgname | tgenabled
---------+--------+-----------
child1 | tg | A
parent | tg | A
(2 rows)DROP TABLE
DROP FUNCTION
----
The patch doesn't start recursion in case 'tgenabled' flag of parent
table is not changes.
Probably vanilla result is more correct.
Thanks for the review and this test case.
I agree that the patch shouldn't have changed that behavior, so I've
fixed the patch so that EnableDisableTrigger() recurses even if the
parent trigger is unchanged.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patchapplication/octet-stream; name=v2-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patchDownload
From 452e6c1522e271d1b02f687bb42e0cf442e0480c Mon Sep 17 00:00:00 2001
From: amitlan <amitlangote09@gmail.com>
Date: Tue, 24 May 2022 12:43:48 +0900
Subject: [PATCH v2] Fix ENABLE/DISABLE TRIGGER to handle recursion correctly
Using ATSimpleRecursion() in ATPrepCmd() to do so as bbb927b4 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
86f575948 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 86f575948 that the
recursion to partition triggers would only occur if
EnableDisableTrigger() had actually changed the trigger. It seems
more apt to recurse to inspect partition triggers even if the
parent's trigger didn't need to be changed.
---
src/backend/commands/tablecmds.c | 131 ++++++++++++++++++++++---
src/backend/commands/trigger.c | 32 +++++-
src/include/commands/trigger.h | 3 +-
src/include/nodes/parsenodes.h | 8 ++
src/test/regress/expected/triggers.out | 40 +++++---
src/test/regress/sql/triggers.sql | 11 ++-
6 files changed, 198 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ef5b34a312..50dd6811c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -579,7 +579,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);
@@ -4756,16 +4757,59 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
/* No command-specific prep needed */
break;
case AT_EnableTrig: /* ENABLE TRIGGER variants */
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableAlwaysTrig:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableAlwaysTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableReplicaTrig:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableReplicaTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableTrigAll:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigAllRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_EnableTrigUser:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_EnableTrigUserRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrig: /* DISABLE TRIGGER variants */
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrigAll:
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigAllRecurse;
+ pass = AT_PASS_MISC;
+ break;
case AT_DisableTrigUser:
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->subtype = AT_DisableTrigUserRecurse;
pass = AT_PASS_MISC;
break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -5106,35 +5150,86 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_EnableTrig: /* ENABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false, false,
+ lockmode);
+ break;
+ case AT_EnableTrigRecurse: /* ENABLE TRIGGER name with recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ON_ORIGIN, false, true,
+ lockmode);
break;
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ALWAYS, false, lockmode);
+ TRIGGER_FIRES_ALWAYS, false, false,
+ lockmode);
+ break;
+ case AT_EnableAlwaysTrigRecurse: /* ENABLE ALWAYS TRIGGER name with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ALWAYS, false, true,
+ lockmode);
break;
case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ON_REPLICA, false, lockmode);
+ TRIGGER_FIRES_ON_REPLICA, false, false,
+ lockmode);
+ break;
+ case AT_EnableReplicaTrigRecurse: /* ENABLE REPLICA TRIGGER name with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_FIRES_ON_REPLICA, false, true,
+ lockmode);
break;
case AT_DisableTrig: /* DISABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false, false,
+ lockmode);
+ break;
+ case AT_DisableTrigRecurse: /* DISABLE TRIGGER name with recursion */
+ ATExecEnableDisableTrigger(rel, cmd->name,
+ TRIGGER_DISABLED, false, true,
+ lockmode);
break;
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false, false,
+ lockmode);
+ break;
+ case AT_EnableTrigAllRecurse: /* ENABLE TRIGGER ALL with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_FIRES_ON_ORIGIN, false, true,
+ lockmode);
break;
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false, false,
+ lockmode);
+ break;
+ case AT_DisableTrigAllRecurse: /* DISABLE TRIGGER ALL with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_DISABLED, false, true,
+ lockmode);
break;
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, true, false,
+ lockmode);
+ break;
+ case AT_EnableTrigUserRecurse: /* ENABLE TRIGGER USER with recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_FIRES_ON_ORIGIN, true, true,
+ lockmode);
break;
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, true, lockmode);
+ TRIGGER_DISABLED, true, false,
+ lockmode);
+ break;
+ case AT_DisableTrigUserRecurse: /* DISABLE TRIGGER USER with
+ * recursion */
+ ATExecEnableDisableTrigger(rel, NULL,
+ TRIGGER_DISABLED, true, true,
+ lockmode);
break;
case AT_EnableRule: /* ENABLE RULE name */
@@ -6147,20 +6242,28 @@ alter_table_type_to_string(AlterTableType cmdtype)
case AT_ReplaceRelOptions:
return NULL; /* not real grammar */
case AT_EnableTrig:
+ case AT_EnableTrigRecurse:
return "ENABLE TRIGGER";
case AT_EnableAlwaysTrig:
+ case AT_EnableAlwaysTrigRecurse:
return "ENABLE ALWAYS TRIGGER";
case AT_EnableReplicaTrig:
+ case AT_EnableReplicaTrigRecurse:
return "ENABLE REPLICA TRIGGER";
case AT_DisableTrig:
+ case AT_DisableTrigRecurse:
return "DISABLE TRIGGER";
case AT_EnableTrigAll:
+ case AT_EnableTrigAllRecurse:
return "ENABLE TRIGGER ALL";
case AT_DisableTrigAll:
+ case AT_DisableTrigAllRecurse:
return "DISABLE TRIGGER ALL";
case AT_EnableTrigUser:
+ case AT_EnableTrigUserRecurse:
return "ENABLE TRIGGER USER";
case AT_DisableTrigUser:
+ case AT_DisableTrigUserRecurse:
return "DISABLE TRIGGER USER";
case AT_EnableRule:
return "ENABLE RULE";
@@ -14691,9 +14794,11 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
*/
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);
+ EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
+ lockmode);
}
/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b8db53b66d..b0416ada94 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1752,6 +1752,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
* 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
@@ -1759,7 +1760,8 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
*/
void
EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode)
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode)
{
Relation tgrel;
int nkeys;
@@ -1825,6 +1827,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, true);
+ int i;
+
+ for (i = 0; i < partdesc->nparts; i++)
+ {
+ Relation part;
+
+ part = relation_open(partdesc->oids[i], lockmode);
+ EnableDisableTrigger(part, NameStr(oldtrig->tgname),
+ fires_when, skip_system, recurse,
+ lockmode);
+ table_close(part, NoLock); /* keep lock till commit */
+ }
+ }
+
InvokeObjectPostAlterHook(TriggerRelationId,
oldtrig->oid, 0);
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 194e8d5bc1..b7b6bd6008 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
extern ObjectAddress renametrig(RenameStmt *stmt);
extern void EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode);
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode);
extern void RelationBuildTriggers(Relation relation);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e2ad761768..4d23c50356 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2281,13 +2281,21 @@ typedef enum AlterTableType
AT_ResetRelOptions, /* RESET (...) -- AM specific parameters */
AT_ReplaceRelOptions, /* replace reloption list in its entirety */
AT_EnableTrig, /* ENABLE TRIGGER name */
+ AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */
+ AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableReplicaTrig, /* ENABLE REPLICA TRIGGER name */
+ AT_EnableReplicaTrigRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrig, /* DISABLE TRIGGER name */
+ AT_DisableTrigRecurse, /* internal to commands/tablecmds.c */
AT_EnableTrigAll, /* ENABLE TRIGGER ALL */
+ AT_EnableTrigAllRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrigAll, /* DISABLE TRIGGER ALL */
+ AT_DisableTrigAllRecurse, /* internal to commands/tablecmds.c */
AT_EnableTrigUser, /* ENABLE TRIGGER USER */
+ AT_EnableTrigUserRecurse, /* internal to commands/tablecmds.c */
AT_DisableTrigUser, /* DISABLE TRIGGER USER */
+ AT_DisableTrigUserRecurse, /* internal to commands/tablecmds.c */
AT_EnableRule, /* ENABLE RULE name */
AT_EnableAlwaysRule, /* ENABLE ALWAYS RULE name */
AT_EnableReplicaRule, /* ENABLE REPLICA RULE name */
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index cd812336f2..6a2ba303cd 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2681,24 +2681,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 83cd00f54f..cb6fc4a90e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1865,10 +1865,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;
--
2.35.3
I agree that the patch shouldn't have changed that behavior, so I've
fixed the patch so that EnableDisableTrigger() recurses even if the
parent trigger is unchanged.
Thanks, I think this patch is ready for committer.
--
With best regards,
Dmitry Koval
Postgres Professional: http://postgrespro.com
On Thu, Jul 14, 2022 at 8:20 PM Dmitry Koval <d.koval@postgrespro.ru> wrote:
I agree that the patch shouldn't have changed that behavior, so I've
fixed the patch so that EnableDisableTrigger() recurses even if the
parent trigger is unchanged.Thanks, I think this patch is ready for committer.
Great, thanks.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On 2022-May-24, Amit Langote wrote:
So, I think we should do something like the attached. A lot of
boilerplate is needed given that the various enable/disable trigger
variants are represented as separate sub-commands (AlterTableCmd
subtypes), which can perhaps be avoided by inventing a
EnableDisableTrigStmt sub-command node that stores (only?) the recurse
flag.
Yeah, I don't know about adding tons of values to that enum just so that
we can use that to hide a boolean inside. Why not add a boolean to the
containing struct? Something like the attached.
We can later use the same thing to undo what happens in in AddColumn,
DropColumn, etc. It all looks pretty strange and confusing to me.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)
Attachments:
triggers-recurse.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e7aef2f6b0..99fcd728dd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -580,7 +580,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);
@@ -4777,8 +4778,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrigAll:
case AT_DisableTrigUser:
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
+ /* Recursion occurs during execution phase */
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->execTimeRecursion = true;
pass = AT_PASS_MISC;
break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -5119,35 +5122,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_EnableTrig: /* ENABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ALWAYS, false, lockmode);
+ TRIGGER_FIRES_ALWAYS, false,
+ cmd->execTimeRecursion,
+ 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->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrig: /* DISABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, true,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, true, lockmode);
+ TRIGGER_DISABLED, true,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableRule: /* ENABLE RULE name */
@@ -14660,9 +14679,11 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
*/
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);
+ EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
+ lockmode);
}
/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b8db53b66d..b0416ada94 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1752,6 +1752,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
* 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
@@ -1759,7 +1760,8 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
*/
void
EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode)
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode)
{
Relation tgrel;
int nkeys;
@@ -1825,6 +1827,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, true);
+ int i;
+
+ for (i = 0; i < partdesc->nparts; i++)
+ {
+ Relation part;
+
+ part = relation_open(partdesc->oids[i], lockmode);
+ EnableDisableTrigger(part, NameStr(oldtrig->tgname),
+ fires_when, skip_system, recurse,
+ lockmode);
+ table_close(part, NoLock); /* keep lock till commit */
+ }
+ }
+
InvokeObjectPostAlterHook(TriggerRelationId,
oldtrig->oid, 0);
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 194e8d5bc1..b7b6bd6008 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
extern ObjectAddress renametrig(RenameStmt *stmt);
extern void EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode);
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode);
extern void RelationBuildTriggers(Relation relation);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 98fe1abaa2..030ea7dfe2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2328,6 +2328,8 @@ 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 execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
+ * recurse to children */
} AlterTableCmd;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 89a34ffbb2..f131405bc7 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2681,24 +2681,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 83cd00f54f..cb6fc4a90e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1865,10 +1865,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;
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
Yeah, I don't know about adding tons of values to that enum just so that
we can use that to hide a boolean inside. Why not add a boolean to the
containing struct? Something like the attached.
I do not think it's a great idea to have ALTER TABLE scribbling on
the source parsetree. That tree could be in plancache and subject
to reuse later.
Mind you, I don't say that we're perfectly clean about this elsewhere.
But there is a pretty hard expectation that the executor doesn't
modify plan trees, and I think the same rule should apply to utility
statements.
regards, tom lane
On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
Yeah, I don't know about adding tons of values to that enum just so that
we can use that to hide a boolean inside. Why not add a boolean to the
containing struct? Something like the attached.I do not think it's a great idea to have ALTER TABLE scribbling on
the source parsetree.
Hmm, I think we already do scribble on the source parse tree even
before this patch, for example, as ATPrepCmd() does for DROP
CONSTRAINT:
if (recurse)
cmd->subtype = AT_DropConstraintRecurse;
That tree could be in plancache and subject
to reuse later.
I see that 7c337b6b527b added 'readOnlyTree' to
standard_ProcessUtility()'s API, I guess, to make any changes that
AlterTable() and underlings make to the input AlterTableStmt be local
to a given execution. Though, maybe that's not really a permission to
add more code that makes such changes?
In this case of needing to remember the inh/recurse flag mentioned in
the original AT command, we could avoid scribbling over the input
AlterTableStmt by setting a new flag in AlteredTableInfo, instead of
AlterTableCmd. AlteredTableInfo has other runtime info about the
relation being altered and perhaps it wouldn't be too bad if it also
stores the inh/recurse flag.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On 2022-Aug-01, Amit Langote wrote:
On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I do not think it's a great idea to have ALTER TABLE scribbling on
the source parsetree.Hmm, I think we already do scribble on the source parse tree even
before this patch, for example, as ATPrepCmd() does for DROP
CONSTRAINT:if (recurse)
cmd->subtype = AT_DropConstraintRecurse;
No, actually nothing scribbles on the parsetree, because ATPrepCmd is
working on a copy of the node, so there's no harm done to the original.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food" (Donald Knuth)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
No, actually nothing scribbles on the parsetree, because ATPrepCmd is
working on a copy of the node, so there's no harm done to the original.
Oh, okay then. Maybe this needs to be noted somewhere?
regards, tom lane
On Tue, Aug 2, 2022 at 3:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Aug-01, Amit Langote wrote:
On Sat, Jul 30, 2022 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I do not think it's a great idea to have ALTER TABLE scribbling on
the source parsetree.Hmm, I think we already do scribble on the source parse tree even
before this patch, for example, as ATPrepCmd() does for DROP
CONSTRAINT:if (recurse)
cmd->subtype = AT_DropConstraintRecurse;No, actually nothing scribbles on the parsetree, because ATPrepCmd is
working on a copy of the node, so there's no harm done to the original.
Oh, I missed this bit in ATPrepCmd():
/*
* 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).
*/
cmd = copyObject(cmd);
That's copying for a different purpose than what Tom mentioned, but
copying nonetheless. Maybe we should modify this comment a bit to
clarify about Tom's concern?
Regarding the patch, I agree that storing the recurse flag rather than
overwriting subtype might be better.
+ bool execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
+ * recurse to children */Might it be better to call this field simply 'recurse'? I think it's
clear from the context and the comment above the flag is to be used
during execution.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
On 2022-Aug-02, Amit Langote wrote:
Regarding the patch, I agree that storing the recurse flag rather than
overwriting subtype might be better.+ bool execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must + * recurse to children */Might it be better to call this field simply 'recurse'? I think it's
clear from the context and the comment above the flag is to be used
during execution.
Yeah, I guess we can do that and also reword the overall ALTER TABLE
comment about recursion. That's in the attached first patch, which is
intended as backpatchable.
The second patch is just to show how we'd rewrite AT_AddColumn to no
longer use the Recurse separate enum value but instead use the ->recurse
flag. This is pretty straightforward and it's a clear net reduction of
code. We can't backpatch this kind of thing of course, both because of
the ABI break (easily fixed) and because potential destabilization
(scary). We can do similar tihngs for the other AT enum values for
recursion. This isn't complete since there are a few other values in
that enum that we should process in this way too; I don't intend it to
push it just yet.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/
Attachments:
triggers-recurse-2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d22dd44712..70b94bbb39 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -580,7 +580,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);
@@ -4057,9 +4058,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
@@ -4067,6 +4066,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.
*
@@ -4487,10 +4492,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
errhint("Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete the pending detach operation."));
/*
- * 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).
+ * 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).
*/
cmd = copyObject(cmd);
@@ -4777,8 +4782,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrigAll:
case AT_DisableTrigUser:
ATSimplePermissions(cmd->subtype, 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 */
@@ -5119,35 +5125,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
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 */
@@ -14660,9 +14682,11 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
*/
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);
+ EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
+ lockmode);
}
/*
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b8db53b66d..b0416ada94 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1752,6 +1752,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
* 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
@@ -1759,7 +1760,8 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
*/
void
EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode)
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode)
{
Relation tgrel;
int nkeys;
@@ -1825,6 +1827,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, true);
+ int i;
+
+ for (i = 0; i < partdesc->nparts; i++)
+ {
+ Relation part;
+
+ part = relation_open(partdesc->oids[i], lockmode);
+ EnableDisableTrigger(part, NameStr(oldtrig->tgname),
+ fires_when, skip_system, recurse,
+ lockmode);
+ table_close(part, NoLock); /* keep lock till commit */
+ }
+ }
+
InvokeObjectPostAlterHook(TriggerRelationId,
oldtrig->oid, 0);
}
diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 194e8d5bc1..b7b6bd6008 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
extern ObjectAddress renametrig(RenameStmt *stmt);
extern void EnableDisableTrigger(Relation rel, const char *tgname,
- char fires_when, bool skip_system, LOCKMODE lockmode);
+ char fires_when, bool skip_system, bool recurse,
+ LOCKMODE lockmode);
extern void RelationBuildTriggers(Relation relation);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 98fe1abaa2..b376031856 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2328,6 +2328,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 89a34ffbb2..f131405bc7 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2681,24 +2681,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 83cd00f54f..cb6fc4a90e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1865,10 +1865,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;
redo-add-column.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 70b94bbb39..ec614a8f14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4929,12 +4929,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
case AT_AddColumn: /* ADD COLUMN */
case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */
address = ATExecAddColumn(wqueue, tab, rel, &cmd,
- false, false,
- lockmode, cur_pass, context);
- break;
- case AT_AddColumnRecurse:
- address = ATExecAddColumn(wqueue, tab, rel, &cmd,
- true, false,
+ cmd->recurse, false,
lockmode, cur_pass, context);
break;
case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */
@@ -6109,7 +6104,6 @@ alter_table_type_to_string(AlterTableType cmdtype)
switch (cmdtype)
{
case AT_AddColumn:
- case AT_AddColumnRecurse:
case AT_AddColumnToView:
return "ADD COLUMN";
case AT_ColumnDefault:
@@ -6670,7 +6664,7 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
ATTypedTableRecursion(wqueue, rel, cmd, lockmode, context);
if (recurse && !is_view)
- cmd->subtype = AT_AddColumnRecurse;
+ cmd->recurse = true;
}
/*
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index b57253463b..b964419f8b 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3355,7 +3355,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
switch (cmd->subtype)
{
case AT_AddColumn:
- case AT_AddColumnRecurse:
{
ColumnDef *def = castNode(ColumnDef, cmd->def);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b376031856..7e0688cdbd 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2236,7 +2236,6 @@ typedef struct AlterTableStmt
typedef enum AlterTableType
{
AT_AddColumn, /* add column */
- AT_AddColumnRecurse, /* internal to commands/tablecmds.c */
AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */
AT_ColumnDefault, /* alter column default */
AT_CookedColumnDefault, /* add a pre-cooked column default */
diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
index bdbe05ceeb..0617a09d2b 100644
--- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
+++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
@@ -114,9 +114,6 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_AddColumn:
strtype = "ADD COLUMN";
break;
- case AT_AddColumnRecurse:
- strtype = "ADD COLUMN (and recurse)";
- break;
case AT_AddColumnToView:
strtype = "ADD COLUMN TO VIEW";
break;
@@ -326,7 +323,10 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
break;
}
- values[0] = CStringGetTextDatum(strtype);
+ if (subcmd->recurse)
+ values[0] = CStringGetTextDatum(psprintf("%s (and recurse)", strtype));
+ else
+ values[0] = CStringGetTextDatum(strtype);
if (OidIsValid(sub->address.objectId))
{
char *objdesc;
On Thu, Aug 4, 2022 at 3:01 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Aug-02, Amit Langote wrote:
Regarding the patch, I agree that storing the recurse flag rather than
overwriting subtype might be better.+ bool execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must + * recurse to children */Might it be better to call this field simply 'recurse'? I think it's
clear from the context and the comment above the flag is to be used
during execution.Yeah, I guess we can do that and also reword the overall ALTER TABLE
comment about recursion. That's in the attached first patch, which is
intended as backpatchable.
Thanks. This one looks good to me.
The second patch is just to show how we'd rewrite AT_AddColumn to no
longer use the Recurse separate enum value but instead use the ->recurse
flag. This is pretty straightforward and it's a clear net reduction of
code. We can't backpatch this kind of thing of course, both because of
the ABI break (easily fixed) and because potential destabilization
(scary). We can do similar tihngs for the other AT enum values for
recursion. This isn't complete since there are a few other values in
that enum that we should process in this way too; I don't intend it to
push it just yet.
I like the idea of removing all AT_*Recurse subtypes in HEAD.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Another point for backpatch: EnableDisableTrigger() changes API, which
is potentially not good. In backbranches I'll keep the function
unchanged and add another function with the added argument,
EnableDisableTriggerNew().
So extensions that want to be compatible with both old and current
versions (assuming any users of that function exist out of core; I
didn't find any) could do something like
#if PG_VERSION_NUM <= 160000
EnableDisableTriggerNew( all args )
#else
EnableDisableTrigger( all args )
#endif
and otherwise they're compatible as compiled today.
Since there are no known users of this interface, it doesn't seem to
warrant any more convenient treatment.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)
On Thu, Aug 4, 2022 at 9:56 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Another point for backpatch: EnableDisableTrigger() changes API, which
is potentially not good. In backbranches I'll keep the function
unchanged and add another function with the added argument,
EnableDisableTriggerNew().
+1
So extensions that want to be compatible with both old and current
versions (assuming any users of that function exist out of core; I
didn't find any) could do something like#if PG_VERSION_NUM <= 160000
EnableDisableTriggerNew( all args )
#else
EnableDisableTrigger( all args )
#endifand otherwise they're compatible as compiled today.
Since there are no known users of this interface, it doesn't seem to
warrant any more convenient treatment.
Makes sense.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
OK, pushed. This soon caused buildfarm to show a failure due to
underspecified ORDER BY, so I just pushed a fix for that too.
Thanks Simon for reporting the problem, and thanks Amit for the patch.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Si quieres ser creativo, aprende el arte de perder el tiempo"
On Fri, Aug 5, 2022 at 6:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
OK, pushed. This soon caused buildfarm to show a failure due to
underspecified ORDER BY, so I just pushed a fix for that too.
Thank you.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com