Looking over the tests added by your (Justin's) patch, there was a
problem checking for non-existance of the CREATE TRIGGER line: it
doesn't work to use "like => {}" (empty), you need to use
"unlike => { everything }". So your test was not catching the fact that
we were, in fact, emitting the undesirable line. I have fixed that
here, and verified that the tests are doing what we wanted them to do.
I also verified the new test in 0001. I was about to add a test for the
legacy inheritance case, but I eventually realized that it was already
being tested by the lines I added in commit bbb927b4db9b.
I have one vote for backpatching 0001 to pg11. Any others? To be
clear: the issue is that if a partitioned table has a disabled trigger,
and a partition is created, the trigger is created as enabled in the
partition. With this patch, the trigger would be created as disabled.
Do people think that that behavior change would be unwelcome?
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)
>From 00708a471aabc3687862bf7151bea75c821c59ee Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 14 Jul 2021 16:27:40 -0400
Subject: [PATCH v4 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..42392f8f41 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 on creation, too
+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..0777c4f50f 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 on creation, too
+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.20.1
>From 07c7368f87ea0b1c75bcef54f075291938f9209e 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 v4 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 | 27 +++++++--
3 files changed, 110 insertions(+), 13 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..9b0723d3ef 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2519,12 +2519,27 @@ my %tests = (
},
},
- # this shouldn't ever get emitted
- 'Creation of row-level trigger in partition' => {
+ 'Disabled trigger on partition is altered' => {
+ 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/^
- \QCREATE TRIGGER test_trigger AFTER INSERT ON dump_test_second_schema.measurement\E
+ \QALTER TABLE dump_test_second_schema.measurement_y2006m3 DISABLE TRIGGER test_trigger;\E
/xm,
- like => {},
+ like => {
+ %full_runs,
+ section_post_data => 1,
+ role => 1,
+ binary_upgrade => 1,
+ },
+ },
+
+ 'Disabled trigger on partition is not created' => {
+ regexp => qr/CREATE TRIGGER test_trigger.*ON dump_test_second_schema/,
+ unlike => { %full_runs },
},
'CREATE TABLE test_fourth_table_zero_col' => {
@@ -3177,9 +3192,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.20.1
