From 487e9861d0cf83e9100ad0d0369147db3ef4ea73 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 18 Mar 2020 18:58:05 -0300 Subject: [PATCH] Enable BEFORE row-level triggers for partitioned tables ... with the limitation that the tuple must remain in the same partition. Reviewed-by: Ashutosh Bapat Discussion: https://postgr.es/m/20200227165158.GA2071@alvherre.pgsql --- doc/src/sgml/ref/create_trigger.sgml | 2 +- doc/src/sgml/trigger.sgml | 3 +- src/backend/commands/tablecmds.c | 3 +- src/backend/commands/trigger.c | 40 +++++++++---- src/backend/partitioning/partdesc.c | 2 +- src/include/utils/reltrigger.h | 1 + src/test/regress/expected/triggers.out | 78 ++++++++++++++++++++++++-- src/test/regress/sql/triggers.sql | 55 +++++++++++++++++- 8 files changed, 161 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml index 3b8f25ea4a..bde3a63f90 100644 --- a/doc/src/sgml/ref/create_trigger.sgml +++ b/doc/src/sgml/ref/create_trigger.sgml @@ -526,7 +526,7 @@ UPDATE OF column_name1 [, column_name2AFTER. + Triggers on partitioned tables may not be INSTEAD OF. diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml index 7d9ad4763a..4a0e74652f 100644 --- a/doc/src/sgml/trigger.sgml +++ b/doc/src/sgml/trigger.sgml @@ -116,8 +116,7 @@ operated on, while row-level AFTER triggers fire at the end of the statement (but before any statement-level AFTER triggers). These types of triggers may only be defined on tables and - foreign tables, not views; BEFORE row-level triggers may not - be defined on partitioned tables. + foreign tables, not views. INSTEAD OF triggers may only be defined on views, and only at row level; they fire immediately as each row in the view is identified as needing to be operated on. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 8c33b67c1b..729025470d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16546,7 +16546,8 @@ CloneRowTriggersToPartition(Relation parent, Relation partition) /* * Complain if we find an unexpected trigger type. */ - if (!TRIGGER_FOR_AFTER(trigForm->tgtype)) + if (!TRIGGER_FOR_BEFORE(trigForm->tgtype) && + !TRIGGER_FOR_AFTER(trigForm->tgtype)) elog(ERROR, "unexpected trigger \"%s\" found", NameStr(trigForm->tgname)); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 513427edf1..ed551ab73a 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 FOR EACH ROW trigger is not supported"), + errdetail("Before executing trigger \"%s\", the 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 executing trigger \"%s\", the 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..e9da4ef983 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,80 @@ 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 v1'); -- works +create trigger t before insert or update or delete on parted + for each row execute function parted_trigfunc(); +insert into parted values (1, 1, 'uno uno v2'); -- fail +ERROR: moving row to another partition during a BEFORE FOR EACH ROW trigger is not supported +DETAIL: Before executing trigger "t", the 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 executing trigger "t", the 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 FOR EACH ROW trigger is not supported +DETAIL: Before executing trigger "t", the 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 executing trigger "t", the 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 v1 v6 and so + parted_1_1 | 1 | 1 | uno uno and so v6 and so +(2 rows) + +drop table parted; +create table parted (a int, b int, c text) partition by list ((a + b)); +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.a = new.a + new.b; + return new; +end; +$$; +create table parted_1 partition of parted for values in (1, 2); +create table parted_2 partition of parted for values in (3, 4); +create trigger t before insert or update on parted + for each row execute function parted_trigfunc(); +insert into parted values (0, 1, 'zero win'); +insert into parted values (1, 1, 'one fail'); +ERROR: moving row to another partition during a BEFORE FOR EACH ROW trigger is not supported +DETAIL: Before executing trigger "t", the row was to be in partition "public.parted_1". +insert into parted values (1, 2, 'two fail'); +ERROR: moving row to another partition during a BEFORE FOR EACH ROW trigger is not supported +DETAIL: Before executing trigger "t", the row was to be in partition "public.parted_2". +select * from parted; + a | b | c +---+---+---------- + 1 | 1 | zero win +(1 row) + +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..80ffbb4b02 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,59 @@ 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 v1'); -- works +create trigger t before insert or update or delete on parted + for each row execute function 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; +create table parted (a int, b int, c text) partition by list ((a + b)); +create or replace function parted_trigfunc() returns trigger language plpgsql as $$ +begin + new.a = new.a + new.b; + return new; +end; +$$; +create table parted_1 partition of parted for values in (1, 2); +create table parted_2 partition of parted for values in (3, 4); +create trigger t before insert or update on parted + for each row execute function parted_trigfunc(); +insert into parted values (0, 1, 'zero win'); +insert into parted values (1, 1, 'one fail'); +insert into parted values (1, 2, 'two fail'); +select * from parted; +drop table parted; +drop function parted_trigfunc(); + -- -- Constraint triggers and partitioned tables create table parted_constr_ancestor (a int, b text)