On 2018-Jun-18, Robert Treat wrote:
> One of the things I was thinking about while reading this thread is
> that the scenario of creating "duplicate" triggers on a table meaning
> two triggers doing the same thing is entirely possible now but we
> don't really do anything to prevent it, which is Ok. I've never seen
> much merit in the argument "people should test" (it assumes a certain
> infallibility that just isn't true) but we've also generally been
> pretty good about exposing what is going on so people can debug this
> type of unexpected behavior.
>
> So +1 for thinking we do need to worry about it. I'm not exactly sure
> how we want to expose that info; with \d+ we list various "Partition
> X:" sections, perhaps adding one for "Partition triggers:" would be
> enough, although I am inclined to think it needs exposure at the \d
> level. One other thing to consider is firing order of said triggers...
> if all parent level triggers fire before child level triggers then the
> above separation is straightforward, but if the execution order is, as
> I suspect, mixed, then it becomes more complicated.
The firing order is alphabetical across the whole set, i.e. parent/child
triggers are interleaved. See the regression test output at the bottom
of this email.
I looked into adding a "Partition triggers" section because it seemed a
nice idea, but looking at the code I realized it's a bit tough because
we already split triggers in "categories": normal triggers, disabled
triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c
line 2795). Since the trigger being in a partition is orthogonal to
that classification, we would end up with eight groups. Not sure that's
great.
The attached patch (catversion bump not included -- beware of server
crash if you run it without initdb'ing) keeps the categories the same.
So with my previous example setup, you can see the duplicate triggers in
psql:
alvherre=# \d child
Table "public.child"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
Partition of: parent FOR VALUES FROM (0) TO (100)
Triggers:
trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
and as soon as you try to drop the second one, you'll learn the truth
about it:
alvherre=# drop trigger trig_p on child;
ERROR: cannot drop trigger trig_p on table child because trigger trig_p on
table parent requires it
SUGERENCIA: You can drop trigger trig_p on table parent instead.
Time: 1,443 ms
I say this is sufficient.
-- Verify that triggers fire in alphabetical order
create table parted_trig (a int) partition by range (a);
create table parted_trig_1 partition of parted_trig for values from (0) to
(1000)
partition by range (a);
create table parted_trig_1_1 partition of parted_trig_1 for values from (0) to
(100);
create table parted_trig_2 partition of parted_trig for values from (1000) to
(2000);
create trigger zzz after insert on parted_trig for each row execute procedure
trigger_notice();
create trigger mmm after insert on parted_trig_1_1 for each row execute
procedure trigger_notice();
create trigger aaa after insert on parted_trig_1 for each row execute procedure
trigger_notice();
create trigger bbb after insert on parted_trig for each row execute procedure
trigger_notice();
create trigger qqq after insert on parted_trig_1_1 for each row execute
procedure trigger_notice();
insert into parted_trig values (50), (1500);
NOTICE: trigger aaa on parted_trig_1_1 AFTER INSERT for ROW
NOTICE: trigger bbb on parted_trig_1_1 AFTER INSERT for ROW
NOTICE: trigger mmm on parted_trig_1_1 AFTER INSERT for ROW
NOTICE: trigger qqq on parted_trig_1_1 AFTER INSERT for ROW
NOTICE: trigger zzz on parted_trig_1_1 AFTER INSERT for ROW
NOTICE: trigger bbb on parted_trig_2 AFTER INSERT for ROW
NOTICE: trigger zzz on parted_trig_2 AFTER INSERT for ROW
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 57519fe8d6..9542856d6a 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -815,11 +815,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
/*
* Build the new pg_trigger tuple.
- *
- * When we're creating a trigger in a partition, we mark it as internal,
- * even though we don't do the isInternal magic in this function. This
- * makes the triggers in partitions identical to the ones in the
- * partitioned tables, except that they are marked internal.
*/
memset(nulls, false, sizeof(nulls));
@@ -829,7 +824,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
values[Anum_pg_trigger_tgfoid - 1] = ObjectIdGetDatum(funcoid);
values[Anum_pg_trigger_tgtype - 1] = Int16GetDatum(tgtype);
values[Anum_pg_trigger_tgenabled - 1] =
CharGetDatum(TRIGGER_FIRES_ON_ORIGIN);
- values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
in_partition);
+ values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
+ values[Anum_pg_trigger_tginpartition - 1] = BoolGetDatum(in_partition);
values[Anum_pg_trigger_tgconstrrelid - 1] =
ObjectIdGetDatum(constrrelid);
values[Anum_pg_trigger_tgconstrindid - 1] = ObjectIdGetDatum(indexOid);
values[Anum_pg_trigger_tgconstraint - 1] =
ObjectIdGetDatum(constraintOid);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 463639208d..04850bd8b4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7444,7 +7444,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int
numTables)
tbinfo->dobj.name);
resetPQExpBuffer(query);
- if (fout->remoteVersion >= 90000)
+ if (fout->remoteVersion >= 110000)
{
/*
* NB: think not to use pretty=true in
pg_get_triggerdef. It
@@ -7458,6 +7458,18 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int
numTables)
"tgenabled, tableoid,
oid "
"FROM
pg_catalog.pg_trigger t "
"WHERE tgrelid =
'%u'::pg_catalog.oid "
+ "AND NOT tgisinternal
AND NOT tginpartition",
+
tbinfo->dobj.catId.oid);
+ }
+ else if (fout->remoteVersion >= 90000)
+ {
+ appendPQExpBuffer(query,
+ "SELECT tgname, "
+
"tgfoid::pg_catalog.regproc AS tgfname, "
+
"pg_catalog.pg_get_triggerdef(oid, false) AS tgdef, "
+ "tgenabled, tableoid,
oid "
+ "FROM
pg_catalog.pg_trigger t "
+ "WHERE tgrelid =
'%u'::pg_catalog.oid "
"AND NOT
tgisinternal",
tbinfo->dobj.catId.oid);
}
diff --git a/src/include/catalog/pg_trigger.h b/src/include/catalog/pg_trigger.h
index 951d7d86e7..02b8718a8f 100644
--- a/src/include/catalog/pg_trigger.h
+++ b/src/include/catalog/pg_trigger.h
@@ -41,6 +41,7 @@ CATALOG(pg_trigger,2620,TriggerRelationId)
char tgenabled; /* trigger's firing
configuration WRT
*
session_replication_role */
bool tgisinternal; /* trigger is system-generated */
+ bool tginpartition; /* trigger is a clone in a partition */
Oid tgconstrrelid; /* constraint's FROM table, if
any */
Oid tgconstrindid; /* constraint's supporting
index, if any */
Oid tgconstraint; /* associated pg_constraint
entry, if any */
