From f13d7ad4c081ede118b0f6c262ad92a49ea9f64c Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 26 Feb 2020 17:34:54 -0300 Subject: [PATCH v2] Enable BEFORE row-level triggers for partitioned tables ... with the limitation that if partitioning columns are changed, the operation is aborted. (The reason for this limitation is that it might require re-routing the tuple, which doesn't work.) --- src/backend/commands/tablecmds.c | 7 ---- src/backend/commands/trigger.c | 40 +++++++++++++------ src/backend/partitioning/partdesc.c | 2 +- src/include/utils/reltrigger.h | 1 + src/test/regress/expected/triggers.out | 53 ++++++++++++++++++++++++-- src/test/regress/sql/triggers.sql | 39 ++++++++++++++++++- 6 files changed, 116 insertions(+), 26 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3eb861bfbf..08ad595596 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16501,13 +16501,6 @@ CloneRowTriggersToPartition(Relation parent, Relation partition) !OidIsValid(trigForm->tgparentid))) continue; - /* - * Complain if we find an unexpected trigger type. - */ - if (!TRIGGER_FOR_AFTER(trigForm->tgtype)) - elog(ERROR, "unexpected trigger \"%s\" found", - NameStr(trigForm->tgname)); - /* Use short-lived context for CREATE TRIGGER */ oldcxt = MemoryContextSwitchTo(perTupCxt); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 513427edf1..98911facad 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -221,18 +221,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, */ if (stmt->row) { - /* - * BEFORE triggers FOR EACH ROW are forbidden, because they would - * allow the user to direct the row to another partition, which - * isn't implemented in the executor. - */ - if (stmt->timing != TRIGGER_TYPE_AFTER) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is a partitioned table", - RelationGetRelationName(rel)), - errdetail("Partitioned tables cannot have BEFORE / FOR EACH ROW triggers."))); - /* * Disallow use of transition tables. * @@ -1658,6 +1646,7 @@ RelationBuildTriggers(Relation relation) build->tgtype = pg_trigger->tgtype; build->tgenabled = pg_trigger->tgenabled; build->tgisinternal = pg_trigger->tgisinternal; + build->tgisclone = OidIsValid(pg_trigger->tgparentid); build->tgconstrrelid = pg_trigger->tgconstrrelid; build->tgconstrindid = pg_trigger->tgconstrindid; build->tgconstraint = pg_trigger->tgconstraint; @@ -1961,6 +1950,8 @@ equalTriggerDescs(TriggerDesc *trigdesc1, TriggerDesc *trigdesc2) return false; if (trig1->tgisinternal != trig2->tgisinternal) return false; + if (trig1->tgisclone != trig2->tgisclone) + return false; if (trig1->tgconstrrelid != trig2->tgconstrrelid) return false; if (trig1->tgconstrindid != trig2->tgconstrindid) @@ -2247,6 +2238,21 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, { ExecForceStoreHeapTuple(newtuple, slot, false); + /* + * After a tuple in a partition goes through a trigger, the user + * could have changed the partition key enough that the tuple + * no longer fits the partition. Verify that. + */ + if (trigger->tgisclone && + !ExecPartitionCheck(relinfo, slot, estate, false)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("moving row to another partition during a BEFORE trigger is not supported"), + errdetail("Before trigger \"%s\", row was to be in partition \"%s.%s\"", + trigger->tgname, + get_namespace_name(RelationGetNamespace(relinfo->ri_RelationDesc)), + RelationGetRelationName(relinfo->ri_RelationDesc)))); + if (should_free) heap_freetuple(oldtuple); @@ -2741,6 +2747,16 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, { ExecForceStoreHeapTuple(newtuple, newslot, false); + if (trigger->tgisclone && + !ExecPartitionCheck(relinfo, newslot, estate, false)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("moving row to another partition during a BEFORE trigger is not supported"), + errdetail("Before trigger \"%s\", row was to be in partition \"%s.%s\"", + trigger->tgname, + get_namespace_name(RelationGetNamespace(relinfo->ri_RelationDesc)), + RelationGetRelationName(relinfo->ri_RelationDesc)))); + /* * If the tuple returned by the trigger / being stored, is the old * row version, and the heap tuple passed to the trigger was diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c index bdda42e40f..e7f23542e8 100644 --- a/src/backend/partitioning/partdesc.c +++ b/src/backend/partitioning/partdesc.c @@ -307,7 +307,7 @@ CreatePartitionDirectory(MemoryContext mcxt) * * The purpose of this function is to ensure that we get the same * PartitionDesc for each relation every time we look it up. In the - * face of current DDL, different PartitionDescs may be constructed with + * face of concurrent DDL, different PartitionDescs may be constructed with * different views of the catalog state, but any single particular OID * will always get the same PartitionDesc for as long as the same * PartitionDirectory is used. diff --git a/src/include/utils/reltrigger.h b/src/include/utils/reltrigger.h index 28df43d833..b22acb034e 100644 --- a/src/include/utils/reltrigger.h +++ b/src/include/utils/reltrigger.h @@ -29,6 +29,7 @@ typedef struct Trigger int16 tgtype; char tgenabled; bool tgisinternal; + bool tgisclone; Oid tgconstrrelid; Oid tgconstrindid; Oid tgconstraint; diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 22e65cc1ec..c51e3acb14 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -1958,10 +1958,6 @@ drop table my_table; create table parted_trig (a int) partition by list (a); create function trigger_nothing() returns trigger language plpgsql as $$ begin end; $$; -create trigger failed before insert or update or delete on parted_trig - for each row execute procedure trigger_nothing(); -ERROR: "parted_trig" is a partitioned table -DETAIL: Partitioned tables cannot have BEFORE / FOR EACH ROW triggers. create trigger failed instead of update on parted_trig for each row execute procedure trigger_nothing(); ERROR: "parted_trig" is a table @@ -2246,6 +2242,55 @@ NOTICE: aasvogel <- woof! NOTICE: trigger parted_trig on parted1_irreg AFTER INSERT for ROW: (a,b)=(3,aasvogel) NOTICE: trigger parted_trig_odd on parted1_irreg AFTER INSERT for ROW: (a,b)=(3,aasvogel) drop table parted_irreg_ancestor; +-- Before triggers and partitions +create table parted (a int, b int, c text) partition by list (a); +create table parted_1 partition of parted for values in (1) + partition by list (b); +create table parted_1_1 partition of parted_1 for values in (1); +create function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.a = new.a + 1; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno'); -- works +create trigger t before insert or update or delete on parted + for each row execute procedure parted_trigfunc(); +insert into parted values (1, 1, 'uno uno v2'); -- fail +ERROR: moving row to another partition during a BEFORE trigger is not supported +DETAIL: Before trigger "t", row was to be in partition "public.parted_1_1" +update parted set c = c || 'v3'; -- fail +ERROR: moving row to another partition during a BEFORE trigger is not supported +DETAIL: Before trigger "t", row was to be in partition "public.parted_1_1" +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.b = new.b + 1; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno v4'); -- fail +ERROR: moving row to another partition during a BEFORE trigger is not supported +DETAIL: Before trigger "t", row was to be in partition "public.parted_1_1" +update parted set c = c || 'v5'; -- fail +ERROR: moving row to another partition during a BEFORE trigger is not supported +DETAIL: Before trigger "t", row was to be in partition "public.parted_1_1" +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.c = new.c || ' and so'; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno'); -- works +update parted set c = c || ' v6'; -- works +select tableoid::regclass, * from parted; + tableoid | a | b | c +------------+---+---+-------------------------- + parted_1_1 | 1 | 1 | uno uno v6 and so + parted_1_1 | 1 | 1 | uno uno and so v6 and so +(2 rows) + +drop table parted; +drop function parted_trigfunc(); -- -- Constraint triggers and partitioned tables create table parted_constr_ancestor (a int, b text) diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 0f61fdf0ea..3d9f14618c 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1348,8 +1348,6 @@ drop table my_table; create table parted_trig (a int) partition by list (a); create function trigger_nothing() returns trigger language plpgsql as $$ begin end; $$; -create trigger failed before insert or update or delete on parted_trig - for each row execute procedure trigger_nothing(); create trigger failed instead of update on parted_trig for each row execute procedure trigger_nothing(); create trigger failed after update on parted_trig @@ -1561,6 +1559,43 @@ insert into parted1_irreg values ('aardwolf', 2); insert into parted_irreg_ancestor values ('aasvogel', 3); drop table parted_irreg_ancestor; +-- Before triggers and partitions +create table parted (a int, b int, c text) partition by list (a); +create table parted_1 partition of parted for values in (1) + partition by list (b); +create table parted_1_1 partition of parted_1 for values in (1); +create function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.a = new.a + 1; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno'); -- works +create trigger t before insert or update or delete on parted + for each row execute procedure parted_trigfunc(); +insert into parted values (1, 1, 'uno uno v2'); -- fail +update parted set c = c || 'v3'; -- fail +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.b = new.b + 1; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno v4'); -- fail +update parted set c = c || 'v5'; -- fail +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.c = new.c || ' and so'; + return new; +end; +$$; +insert into parted values (1, 1, 'uno uno'); -- works +update parted set c = c || ' v6'; -- works +select tableoid::regclass, * from parted; + +drop table parted; +drop function parted_trigfunc(); + -- -- Constraint triggers and partitioned tables create table parted_constr_ancestor (a int, b text) -- 2.20.1