On 2020-Oct-27, Justin Pryzby wrote: > I think either way could be ok - if you assume that the trigger was disabled > with ONLY, then it'd make sense to restore it with ONLY, but I think it's at > least as common to ALTER TABLE [*]. It might look weird to the user if they > used ALTER TABLE ONLY and the pg_dump output uses ALTER TABLE for that table, > and then again for all its children (or vice versa). But it's fine as long as > the state is correctly restored. > > There are serveral issues: > - fail to preserve childs' tgenabled in CREATE TABLE PARTITION OF; > - fail to preserve childs' tgenabled in pg_dump; > - fail to preserve childs' comments in pg_dump; > > I'm going step away from this patch at least for awhile, so I'm attaching my > latest in case it's useful.
Here's a new cut of this series. I used your pg_dump patch, but I blank out the CREATE TRIGGER query before stashing the ALTER TRIGGER; otherwise the dump has an error at restore time (because the trigger is created again on the partition, but it already exists because it's been created for the parent). Also, the new query has the "OR tgenabled <>" test only if the table is a partition; and apply this new query only in 11 and 12; keep 9.x-10 unchanged, because it cannot possibly match anything. I tested this by creating 10k tables with one trigger each (no partitioned tables). Total time to dump is the same as before. I was concerned that because the query now has two LEFT JOINs it would be slower; but it seems to be only marginally so. I'm thinking to apply my patch that changes the server behavior only to 14 and up. I could be persuaded to backpatch all the way to 11, if anybody supports the idea. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
>From 29b595d02af124900d9f1e9655f6fe3d2725bfd1 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Fri, 16 Oct 2020 10:58:54 -0300 Subject: [PATCH v3 1/2] Preserve firing-on state when cloning row triggers to partitions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When triggers are cloned from partitioned tables to their partitions, the 'tgenabled' flag (origin/replica/always/disable) was not propagated. Make it so that the flag on the trigger on partition is initially set to the same value as on the partitioned table. Add a test case to verify the behavior. Backpatch to 14. The original behavior, which appeared in pg11 with commit 86f575948c77 doesn't really make much sense, but it seems risky to change it on established branches. Author: Álvaro Herrera <alvhe...@alvh.no-ip.org> Reported-by: Justin Pryzby <pry...@telsasoft.com> Discussion: https://postgr.es/m/20200930223450.ga14...@telsasoft.com --- src/backend/commands/tablecmds.c | 4 +- src/backend/commands/trigger.c | 30 +++++++++++--- src/include/commands/trigger.h | 5 +++ src/test/regress/expected/triggers.out | 56 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 32 +++++++++++++++ 5 files changed, 119 insertions(+), 8 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 96375814a8..dd2aefe1f3 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -17736,10 +17736,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition) trigStmt->initdeferred = trigForm->tginitdeferred; trigStmt->constrrel = NULL; /* passed separately */ - CreateTrigger(trigStmt, NULL, RelationGetRelid(partition), + CreateTriggerFiringOn(trigStmt, NULL, RelationGetRelid(partition), trigForm->tgconstrrelid, InvalidOid, InvalidOid, trigForm->tgfoid, trigForm->oid, qual, - false, true); + false, true, trigForm->tgenabled); MemoryContextSwitchTo(oldcxt); MemoryContextReset(perTupCxt); diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 952c8d582a..6d4b7ee92a 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -151,6 +151,24 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid, Oid funcoid, Oid parentTriggerOid, Node *whenClause, bool isInternal, bool in_partition) +{ + return + CreateTriggerFiringOn(stmt, queryString, relOid, refRelOid, + constraintOid, indexOid, funcoid, + parentTriggerOid, whenClause, isInternal, + in_partition, TRIGGER_FIRES_ON_ORIGIN); +} + +/* + * Like the above; additionally the firing condition + * (always/origin/replica/disabled) can be specified. + */ +ObjectAddress +CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, + Oid relOid, Oid refRelOid, Oid constraintOid, + Oid indexOid, Oid funcoid, Oid parentTriggerOid, + Node *whenClause, bool isInternal, bool in_partition, + char trigger_fires_when) { int16 tgtype; int ncolumns; @@ -849,7 +867,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, CStringGetDatum(trigname)); 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_tgenabled - 1] = trigger_fires_when; values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal || in_partition); values[Anum_pg_trigger_tgconstrrelid - 1] = ObjectIdGetDatum(constrrelid); values[Anum_pg_trigger_tgconstrindid - 1] = ObjectIdGetDatum(indexOid); @@ -1196,11 +1214,11 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, map_partition_varattnos((List *) qual, PRS2_NEW_VARNO, childTbl, rel); - CreateTrigger(childStmt, queryString, - partdesc->oids[i], refRelOid, - InvalidOid, indexOnChild, - funcoid, trigoid, qual, - isInternal, true); + CreateTriggerFiringOn(childStmt, queryString, + partdesc->oids[i], refRelOid, + InvalidOid, indexOnChild, + funcoid, trigoid, qual, + isInternal, true, trigger_fires_when); table_close(childTbl, NoLock); diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 9e557cfbce..6474237a95 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -154,6 +154,11 @@ extern ObjectAddress CreateTrigger(CreateTrigStmt *stmt, const char *queryString Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid, Oid funcoid, Oid parentTriggerOid, Node *whenClause, bool isInternal, bool in_partition); +extern ObjectAddress CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, + Oid relOid, Oid refRelOid, Oid constraintOid, + Oid indexOid, Oid funcoid, Oid parentTriggerOid, + Node *whenClause, bool isInternal, bool in_partition, + char trigger_fires_when); extern void RemoveTriggerById(Oid trigOid); extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok); diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index e8af9a9589..5ee7acc909 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2661,6 +2661,62 @@ select tgrelid::regclass, tgname, tgenabled from pg_trigger (2 rows) drop table parent, child1; +-- Verify that firing state propagates correctly +CREATE TABLE trgfire (i int) PARTITION BY RANGE (i); +CREATE TABLE trgfire1 PARTITION OF trgfire FOR VALUES FROM (1) TO (10); +CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql + AS $$ begin raise exception 'except'; end $$; +CREATE TRIGGER tg AFTER INSERT ON trgfire FOR EACH ROW EXECUTE FUNCTION tgf(); +INSERT INTO trgfire VALUES (1); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +ALTER TABLE trgfire DISABLE TRIGGER tg; +INSERT INTO trgfire VALUES (1); +CREATE TABLE trgfire2 PARTITION OF trgfire FOR VALUES FROM (10) TO (20); +INSERT INTO trgfire VALUES (11); +CREATE TABLE trgfire3 (LIKE trgfire); +ALTER TABLE trgfire ATTACH PARTITION trgfire3 FOR VALUES FROM (20) TO (30); +INSERT INTO trgfire VALUES (21); +CREATE TABLE trgfire4 PARTITION OF trgfire FOR VALUES FROM (30) TO (40) PARTITION BY LIST (i); +CREATE TABLE trgfire4_30 PARTITION OF trgfire4 FOR VALUES IN (30); +INSERT INTO trgfire VALUES (30); +CREATE TABLE trgfire5 (LIKE trgfire) PARTITION BY LIST (i); +CREATE TABLE trgfire5_40 PARTITION OF trgfire5 FOR VALUES IN (40); +ALTER TABLE trgfire ATTACH PARTITION trgfire5 FOR VALUES FROM (40) TO (50); +INSERT INTO trgfire VALUES (40); +SELECT tgrelid::regclass, tgenabled FROM pg_trigger + WHERE tgrelid::regclass IN (SELECT oid from pg_class where relname LIKE 'trgfire%') + ORDER BY tgrelid::regclass::text; + tgrelid | tgenabled +-------------+----------- + trgfire | D + trgfire1 | D + trgfire2 | D + trgfire3 | D + trgfire4 | D + trgfire4_30 | D + trgfire5 | D + trgfire5_40 | D +(8 rows) + +ALTER TABLE trgfire ENABLE TRIGGER tg; +INSERT INTO trgfire VALUES (1); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +INSERT INTO trgfire VALUES (11); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +INSERT INTO trgfire VALUES (21); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +INSERT INTO trgfire VALUES (30); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +INSERT INTO trgfire VALUES (40); +ERROR: except +CONTEXT: PL/pgSQL function tgf() line 1 at RAISE +DROP TABLE trgfire; +DROP FUNCTION tgf(); -- -- Test the interaction between transition tables and both kinds of -- inheritance. We'll dump the contents of the transition tables in a diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index b50f500045..0f9c718ab1 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1836,6 +1836,38 @@ select tgrelid::regclass, tgname, tgenabled from pg_trigger order by tgrelid::regclass::text; drop table parent, child1; +-- Verify that firing state propagates correctly +CREATE TABLE trgfire (i int) PARTITION BY RANGE (i); +CREATE TABLE trgfire1 PARTITION OF trgfire FOR VALUES FROM (1) TO (10); +CREATE OR REPLACE FUNCTION tgf() RETURNS trigger LANGUAGE plpgsql + AS $$ begin raise exception 'except'; end $$; +CREATE TRIGGER tg AFTER INSERT ON trgfire FOR EACH ROW EXECUTE FUNCTION tgf(); +INSERT INTO trgfire VALUES (1); +ALTER TABLE trgfire DISABLE TRIGGER tg; +INSERT INTO trgfire VALUES (1); +CREATE TABLE trgfire2 PARTITION OF trgfire FOR VALUES FROM (10) TO (20); +INSERT INTO trgfire VALUES (11); +CREATE TABLE trgfire3 (LIKE trgfire); +ALTER TABLE trgfire ATTACH PARTITION trgfire3 FOR VALUES FROM (20) TO (30); +INSERT INTO trgfire VALUES (21); +CREATE TABLE trgfire4 PARTITION OF trgfire FOR VALUES FROM (30) TO (40) PARTITION BY LIST (i); +CREATE TABLE trgfire4_30 PARTITION OF trgfire4 FOR VALUES IN (30); +INSERT INTO trgfire VALUES (30); +CREATE TABLE trgfire5 (LIKE trgfire) PARTITION BY LIST (i); +CREATE TABLE trgfire5_40 PARTITION OF trgfire5 FOR VALUES IN (40); +ALTER TABLE trgfire ATTACH PARTITION trgfire5 FOR VALUES FROM (40) TO (50); +INSERT INTO trgfire VALUES (40); +SELECT tgrelid::regclass, tgenabled FROM pg_trigger + WHERE tgrelid::regclass IN (SELECT oid from pg_class where relname LIKE 'trgfire%') + ORDER BY tgrelid::regclass::text; +ALTER TABLE trgfire ENABLE TRIGGER tg; +INSERT INTO trgfire VALUES (1); +INSERT INTO trgfire VALUES (11); +INSERT INTO trgfire VALUES (21); +INSERT INTO trgfire VALUES (30); +INSERT INTO trgfire VALUES (40); +DROP TABLE trgfire; +DROP FUNCTION tgf(); -- -- Test the interaction between transition tables and both kinds of -- 2.30.2
>From 0730f32f7608310c1cc689fa587dd439e66fc6b7 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Wed, 14 Jul 2021 13:40:02 -0400 Subject: [PATCH v3 2/2] Fix pg_dump for triggers with changed enabled flag --- src/bin/pg_dump/pg_dump.c | 95 +++++++++++++++++++++++++++++--- src/bin/pg_dump/pg_dump.h | 1 + src/bin/pg_dump/t/002_pg_dump.pl | 26 +++++++-- 3 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 912144c43e..c1f8f3ec9a 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7998,6 +7998,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) i_tgconstrrelid, i_tgconstrrelname, i_tgenabled, + i_tgisinternal, i_tgdeferrable, i_tginitdeferred, i_tgdef; @@ -8016,18 +8017,62 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) tbinfo->dobj.name); resetPQExpBuffer(query); - if (fout->remoteVersion >= 90000) + if (fout->remoteVersion >= 130000) { /* * NB: think not to use pretty=true in pg_get_triggerdef. It * could result in non-forward-compatible dumps of WHEN clauses * due to under-parenthesization. + * + * NB: We need to see tginternal triggers in partitions, in case + * the tgenabled flag has been changed from the parent. */ appendPQExpBuffer(query, - "SELECT tgname, " - "tgfoid::pg_catalog.regproc AS tgfname, " - "pg_catalog.pg_get_triggerdef(oid, false) AS tgdef, " - "tgenabled, tableoid, oid " + "SELECT t.tgname, " + "t.tgfoid::pg_catalog.regproc AS tgfname, " + "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, " + "t.tgenabled, t.tableoid, t.oid, t.tgisinternal " + "FROM pg_catalog.pg_trigger t " + "LEFT JOIN pg_catalog.pg_trigger u ON u.oid = t.tgparentid " + "WHERE t.tgrelid = '%u'::pg_catalog.oid " + "AND (NOT t.tgisinternal OR t.tgenabled != u.tgenabled)", + tbinfo->dobj.catId.oid); + } + else if (fout->remoteVersion >= 110000) + { + /* + * NB: think not to use pretty=true in pg_get_triggerdef. It + * could result in non-forward-compatible dumps of WHEN clauses + * due to under-parenthesization. + * + * NB: We need to see tginternal triggers in partitions, in case + * the tgenabled flag has been changed from the parent. No + * tgparentid in version 11-12, so we have to match them via + * pg_depend. + */ + appendPQExpBuffer(query, + "SELECT t.tgname, " + "t.tgfoid::pg_catalog.regproc AS tgfname, " + "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, " + "t.tgenabled, t.tableoid, t.oid, t.tgisinternal " + "FROM pg_catalog.pg_trigger t " + "LEFT JOIN pg_catalog.pg_depend AS d ON " + " d.classid = 'pg_catalog.pg_trigger'::pg_catalog.regclass AND " + " d.refclassid = 'pg_catalog.pg_trigger'::pg_catalog.regclass AND d.objid = t.oid " + "LEFT JOIN pg_catalog.pg_trigger AS pt ON pt.oid = refobjid " + "WHERE t.tgrelid = '%u'::pg_catalog.oid " + "AND (NOT t.tgisinternal%s)", + tbinfo->dobj.catId.oid, + tbinfo->ispartition ? + " OR t.tgenabled != pt.tgenabled" : ""); + } + else if (fout->remoteVersion >= 90000) + { + appendPQExpBuffer(query, + "SELECT t.tgname, " + "t.tgfoid::pg_catalog.regproc AS tgfname, " + "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, " + "t.tgenabled, t.tableoid, t.oid, t.tgisinternal " "FROM pg_catalog.pg_trigger t " "WHERE tgrelid = '%u'::pg_catalog.oid " "AND NOT tgisinternal", @@ -8090,6 +8135,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) i_tgconstrrelid = PQfnumber(res, "tgconstrrelid"); i_tgconstrrelname = PQfnumber(res, "tgconstrrelname"); i_tgenabled = PQfnumber(res, "tgenabled"); + i_tgisinternal = PQfnumber(res, "tgisinternal"); i_tgdeferrable = PQfnumber(res, "tgdeferrable"); i_tginitdeferred = PQfnumber(res, "tginitdeferred"); i_tgdef = PQfnumber(res, "tgdef"); @@ -8101,14 +8147,18 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) for (j = 0; j < ntups; j++) { + Oid trigoid = atooid(PQgetvalue(res, j, i_oid)); + bool tgisinternal = *(PQgetvalue(res, j, i_tgisinternal)) == 't'; + tginfo[j].dobj.objType = DO_TRIGGER; tginfo[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, i_tableoid)); - tginfo[j].dobj.catId.oid = atooid(PQgetvalue(res, j, i_oid)); + tginfo[j].dobj.catId.oid = trigoid; AssignDumpId(&tginfo[j].dobj); tginfo[j].dobj.name = pg_strdup(PQgetvalue(res, j, i_tgname)); tginfo[j].dobj.namespace = tbinfo->dobj.namespace; tginfo[j].tgtable = tbinfo; tginfo[j].tgenabled = *(PQgetvalue(res, j, i_tgenabled)); + tginfo[j].tgisinternal = tgisinternal; if (i_tgdef >= 0) { tginfo[j].tgdef = pg_strdup(PQgetvalue(res, j, i_tgdef)); @@ -17799,7 +17849,38 @@ dumpTrigger(Archive *fout, const TriggerInfo *tginfo) "pg_catalog.pg_trigger", "TRIGGER", trigidentity->data); - if (tginfo->tgenabled != 't' && tginfo->tgenabled != 'O') + if (tginfo->tgisinternal) + { + /* + * Triggers marked internal only appear here because their 'tgenabled' + * flag differs from its parent's. The trigger is created already, + * so remove the CREATE and replace it with an ALTER. + */ + resetPQExpBuffer(query); + appendPQExpBuffer(query, "\nALTER %sTABLE %s ", + tbinfo->relkind == RELKIND_FOREIGN_TABLE ? "FOREIGN " : "", + fmtQualifiedDumpable(tbinfo)); + switch (tginfo->tgenabled) + { + case 'f': + case 'D': + appendPQExpBufferStr(query, "DISABLE"); + break; + case 't': + case 'O': + appendPQExpBufferStr(query, "ENABLE"); + break; + case 'R': + appendPQExpBufferStr(query, "ENABLE REPLICA"); + break; + case 'A': + appendPQExpBufferStr(query, "ENABLE ALWAYS"); + break; + } + appendPQExpBuffer(query, " TRIGGER %s;\n", + fmtId(tginfo->dobj.name)); + } + else if (tginfo->tgenabled != 't' && tginfo->tgenabled != 'O') { appendPQExpBuffer(query, "\nALTER %sTABLE %s ", tbinfo->relkind == RELKIND_FOREIGN_TABLE ? "FOREIGN " : "", diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index efb8c30e71..f5e170e0db 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -425,6 +425,7 @@ typedef struct _triggerInfo Oid tgconstrrelid; char *tgconstrrelname; char tgenabled; + bool tgisinternal; bool tgdeferrable; bool tginitdeferred; char *tgdef; diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index 448b1be26c..57f909f2c4 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -2519,10 +2519,28 @@ my %tests = ( }, }, - # this shouldn't ever get emitted + 'Partition measurement_y2006m3 creation - disabled child trigger' => { + create_order => 93, + create_sql => + 'CREATE TABLE dump_test_second_schema.measurement_y2006m3 + PARTITION OF dump_test.measurement + FOR VALUES FROM (\'2006-03-01\') TO (\'2006-04-01\'); + ALTER TABLE dump_test_second_schema.measurement_y2006m3 DISABLE TRIGGER test_trigger', + regexp => qr/^ + \QALTER TABLE dump_test_second_schema.measurement_y2006m3 DISABLE TRIGGER test_trigger;\E + /xm, + like => { + %full_runs, + section_post_data => 1, + role => 1, + binary_upgrade => 1, + }, + }, + + # this shouldn't get emitted except for triggers with altered disabled/enabled 'Creation of row-level trigger in partition' => { regexp => qr/^ - \QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test_second_schema.measurement\E + \QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test_second_schema.measurement_y2006m2\E /xm, like => {}, }, @@ -3177,9 +3195,9 @@ my %tests = ( }, 'GRANT SELECT ON TABLE measurement_y2006m2' => { - create_order => 92, + create_order => 94, create_sql => 'GRANT SELECT ON - TABLE dump_test_second_schema.measurement_y2006m2 + TABLE dump_test_second_schema.measurement_y2006m2, dump_test_second_schema.measurement_y2006m3 TO regress_dump_test_role;', regexp => qr/^\QGRANT SELECT ON TABLE dump_test_second_schema.measurement_y2006m2 TO regress_dump_test_role;\E/m, -- 2.30.2