Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Fri, Jul 16, 2021 at 06:01:12PM -0400, Alvaro Herrera wrote: > On 2021-Jul-16, Justin Pryzby wrote: > > CREATE TABLE p(i int) PARTITION BY RANGE(i); > > CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (1)TO(2); > > CREATE FUNCTION foo() returns trigger LANGUAGE plpgsql AS $$begin end$$; > > CREATE TRIGGER x AFTER DELETE ON p1 EXECUTE FUNCTION foo(); > > CREATE TRIGGER x AFTER DELETE ON p EXECUTE FUNCTION foo(); > > Hmm, interesting -- those statement triggers are not cloned, so what is > going on here is just that the psql query to show them is tripping on > its shoelaces ... I'll try to find a fix. > > I *think* the problem is that the query matches triggers by name and > parent/child relationship; we're missing to ignore triggers by tgtype. > It's not great design that tgtype is a bitmask of unrelated flags ... I see it's the subquery Amit wrote and proposed here: https://www.postgresql.org/message-id/CA+HiwqEiMe0tCOoPOwjQrdH5fxnZccMR7oeW=f9fmgszjqb...@mail.gmail.com .. and I realize that I've accidentally succeeded in breaking what I first attempted to break 15 months ago: On Mon, Apr 20, 2020 at 02:57:40PM -0500, Justin Pryzby wrote: > I'm happy to see that this doesn't require a recursive cte, at least. > I was trying to think how to break it by returning multiple results or results > out of order, but I think that can't happen. If you assume that pg_partition_ancestors returns its results in order, I think you can fix it by adding LIMIT 1. Otherwise I think you need a recursive CTE, as I'd feared. Note also that I'd sent a patch to add newlines, to make psql -E look pretty. v6-0001-fixups-c33869cc3bfc42bce822251f2fa1a2a346f86cc5.patch -- Justin
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2021-Jul-16, Justin Pryzby wrote: > CREATE TABLE p(i int) PARTITION BY RANGE(i); > CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (1)TO(2); > CREATE FUNCTION foo() returns trigger LANGUAGE plpgsql AS $$begin end$$; > CREATE TRIGGER x AFTER DELETE ON p1 EXECUTE FUNCTION foo(); > CREATE TRIGGER x AFTER DELETE ON p EXECUTE FUNCTION foo(); Hmm, interesting -- those statement triggers are not cloned, so what is going on here is just that the psql query to show them is tripping on its shoelaces ... I'll try to find a fix. I *think* the problem is that the query matches triggers by name and parent/child relationship; we're missing to ignore triggers by tgtype. It's not great design that tgtype is a bitmask of unrelated flags ... -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
I noticed that in pg_dump --clean, we were still emitting DROP lines for
the triggers in the partitions, which raises errors; so I emptied that
too.
On 2021-Jul-14, Alvaro Herrera wrote:
> 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.
Actually, that is wrong; unlike is there just to be able to subtract
dumps from the set that is going to be searched for the regexp, for when
you want to use one of the hashes (%full_runs) but ignore a few of
those.
> I have fixed that here, and verified that the tests are doing what we
> wanted them to do.
... and I added some more tests, for pg_upgrade.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Fri, Jul 16, 2021 at 02:15:26PM -0400, Tom Lane wrote: > Justin Pryzby writes: > > I think there's still an issue that comments on child triggers aren't > > preserved, right ? > > Do we care? That seems like messing with a system-internal object. > In general we won't promise to preserve the results of doing so. It's fine if that's the conclusion. Back in October, that seemed like one too many things misbehaving, which led me to walk away from the patch and re-orient. I was going re-check the behavior, but instead hit another bug: CREATE TABLE p(i int) PARTITION BY RANGE(i); CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (1)TO(2); CREATE FUNCTION foo() returns trigger LANGUAGE plpgsql AS $$begin end$$; CREATE TRIGGER x AFTER DELETE ON p1 EXECUTE FUNCTION foo(); CREATE TRIGGER x AFTER DELETE ON p EXECUTE FUNCTION foo(); \d p1 2021-07-16 14:30:12.371 CDT client backend[6252] psql ERROR: more than one row returned by a subquery used as an expression 2021-07-16 14:30:12.371 CDT client backend[6252] psql STATEMENT: SELECT t.tgname, pg_catalog.pg_get_triggerdef(t.oid, true), t.tgenabled, t.tgisinternal, (SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass FROM pg_catalog.pg_trigger AS u, pg_catalog.pg_partition_ancestors(t.tgrelid) AS a WHERE u.tgname = t.tgname AND u.tgrelid = a.relid AND u.tgparentid = 0) AS parent FROM pg_catalog.pg_trigger t WHERE t.tgrelid = '37718' AND (NOT t.tgisinternal OR (t.tgisinternal AND t.tgenabled = 'D') OR EXISTS (SELECT 1 FROM pg_catalog.pg_depend WHERE objid = t.oid AND refclassid = 'pg_catalog.pg_trigger'::pg_catalog.regclass)) ORDER BY 1; ERROR: more than one row returned by a subquery used as an expression The tgenabled issue was a contributing factor which led us to stop using inherited triggers, so I'm not very motivated to dig into it. -- Justin
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
Justin Pryzby writes: > I think there's still an issue that comments on child triggers aren't > preserved, right ? Do we care? That seems like messing with a system-internal object. In general we won't promise to preserve the results of doing so. regards, tom lane
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
Thanks for handling this. I think there's still an issue that comments on child triggers aren't preserved, right ? -- Justin
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
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
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
Reported-by: Justin Pryzby
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,
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Wed, Jul 14, 2021 at 11:02 AM Alvaro Herrera wrote: > 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" > Hi, Alvaro: It would be nice if this is backported to PG 11+ Thanks
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
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
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
Reported-by: Justin Pryzby
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
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Wed, Oct 21, 2020 at 02:02:37PM -0300, Alvaro Herrera wrote:
> On 2020-Oct-21, Justin Pryzby wrote:
>
> > I came up with this, which probably needs more than a little finesse.
>
> Hmm, there are two important changes needed on this: 1) it must not emit
> CREATE lines for the child triggers; only the ALTER TABLE ONLY
> lines to set disable state on the partition are needed. 2)
> tgparentid does not exist prior to pg13, so you need some additional
> trick to cover that case.
Thanks for looking.
> Also, I think the multipartition case is broken: if grandparent has
> trigger enabled, parent has trigger disabled and child trigger set to
> always, is that dumped correctly? I think the right way to do this is
> change only the partitions that differ from the topmost partitioned
> table -- not their immediate parents; and use ONLY to ensure they don't
> affect downstream children.
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.
--
Justin
>From 15cb95df4e3771f0bb3fe2f1fd8dfb94fe3f7a50 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 20 Oct 2020 23:19:07 -0500
Subject: [PATCH v2] pg_dump: output DISABLE/ENABLE for child triggers ..
..if their state does not match their parent
---
src/bin/pg_dump/pg_dump.c| 62 +++-
src/bin/pg_dump/pg_dump.h| 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 26 +++---
3 files changed, 77 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ff45e3fb8c..40844fa1e7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7811,6 +7811,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
i_tgconstrrelid,
i_tgconstrrelname,
i_tgenabled,
+i_tgisinternal,
i_tgdeferrable,
i_tginitdeferred,
i_tgdef;
@@ -7829,21 +7830,46 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
tbinfo->dobj.name);
resetPQExpBuffer(query);
- if (fout->remoteVersion >= 9)
+ if (fout->remoteVersion >= 13000)
{
/*
* 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.
+ * Use tgparentid, which is available since v13.
*/
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 "
- "WHERE tgrelid = '%u'::pg_catalog.oid "
- "AND NOT tgisinternal",
+ "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 >= 9)
+ {
+ /*
+ * 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.
+ * The pg_depend joins handle v11-v12, which allow inherited row
+ * triggers, but did not have tgparentid. And do nothing between v9-v10.
+ */
+ 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_trigger'::pg_catalog.regclass AND "
+ " d.refclassid = 'pg_trigger'::pg_catalog.regclass AND d.objid = t.oid "
+ "LEFT JOIN pg_catalog.pg_trigger AS u ON u.oid = refobjid "
+ "WHERE t.tgrelid = '%u'::pg_catalog.oid "
+ "AND (NOT t.tgisinternal OR t.tgenabled != u.tgenabled)",
tbinfo->dobj.catId.oid);
}
else if (fout->remoteVersion >= 80300)
@@ -7903,6 +7929,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
i_tgconstrrelid = PQfnumber(res,
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Oct-21, Justin Pryzby wrote: > I came up with this, which probably needs more than a little finesse. Hmm, there are two important changes needed on this: 1) it must not emit CREATE lines for the child triggers; only the ALTER TABLE ONLY lines to set disable state on the partition are needed. 2) tgparentid does not exist prior to pg13, so you need some additional trick to cover that case. Also, I think the multipartition case is broken: if grandparent has trigger enabled, parent has trigger disabled and child trigger set to always, is that dumped correctly? I think the right way to do this is change only the partitions that differ from the topmost partitioned table -- not their immediate parents; and use ONLY to ensure they don't affect downstream children. Change 1 also means that the test with the "this shouldn't ever get emitted" comment remains unchanged. I'm not sure how to tackle change 2. You need to search pg_depend for entries with classid=pg_trigger and refclass=pg_trigger ... (commit 1fa846f1c9af might give some clue)
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Tue, Oct 20, 2020 at 09:54:53PM -0300, Alvaro Herrera wrote:
> On 2020-Oct-20, Justin Pryzby wrote:
> > On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote:
> > > Alvaro Herrera writes:
> > > > Hmm, next question: should we backpatch a fix for this? (This applies
> > > > all the way back to 11.) If we do, then we would change behavior of
> > > > partition creation. It's hard to see that the current behavior is
> > > > desirable ... and I think anybody who would have come across this, would
> > > > wish it behaved the other way. But still -- it would definitely be a
> > > > behavior change.
> > >
> > > The behavior change seems like it'd be an improvement in a vacuum,
> > > but I wonder how it would interact with catalog contents left behind
> > > by the old misbehavior. Also, would we expect pg_dump to try to do
> > > anything to clean up the mess? If so, allowing a back branch to have
> > > had more than one behavior would complicate that greatly.
> >
> > I don't think there's a problem with catalog content ?
> > I think it's fine if there's an enabled child trigger inheriting from a
> > disabled parent? This changes the initial tgenabled for new partitions.
>
> I don't think we'd need to do anything special here ... particularly
> considering the discovery that pg_dump does not preserve the disable
> status of trigger on partitions:
>
> > However...it looks like pg_dump should ALTER the child trigger state if it
> > differ from its parent. Or maybe it needs to CREATE child triggers with the
> > proper state before attaching the child table ?
>
> I guess *something* needs to be done, but I'm not clear on what it is.
> Creating the trigger on partition beforehand does not work: an error is
> raised on attach that the trigger already exists.
>
> The only way I see to do this, is to have pg_dump extract tgenabled for
I came up with this, which probably needs more than a little finesse.
--
Justin
>From 465fba070986774e8bf4ec911986cc97dd211b20 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 20 Oct 2020 23:19:07 -0500
Subject: [PATCH v1] pg_dump: output DISABLE/ENABLE for child triggers ..
..if their state does not match their parent
---
src/bin/pg_dump/pg_dump.c| 36 +---
src/bin/pg_dump/pg_dump.h| 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 26 +++
3 files changed, 52 insertions(+), 11 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ff45e3fb8c..c4b7046cac 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7792,77 +7792,79 @@ getRules(Archive *fout, int *numRules)
* does get entered into the DumpableObject tables.
*/
void
getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
{
int i,
j;
PQExpBuffer query = createPQExpBuffer();
PGresult *res;
TriggerInfo *tginfo;
int i_tableoid,
i_oid,
i_tgname,
i_tgfname,
i_tgtype,
i_tgnargs,
i_tgargs,
i_tgisconstraint,
i_tgconstrname,
i_tgconstrrelid,
i_tgconstrrelname,
i_tgenabled,
+i_tgisinternal,
i_tgdeferrable,
i_tginitdeferred,
i_tgdef;
int ntups;
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = [i];
if (!tbinfo->hastriggers ||
!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
pg_log_info("reading triggers for table \"%s.%s\"",
tbinfo->dobj.namespace->dobj.name,
tbinfo->dobj.name);
resetPQExpBuffer(query);
if (fout->remoteVersion >= 9)
{
/*
* 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.
*/
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 "
- "WHERE tgrelid = '%u'::pg_catalog.oid "
- "AND NOT tgisinternal",
+ "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 >= 80300)
{
/*
* We ignore triggers that are tied to a foreign-key constraint
*/
appendPQExpBuffer(query,
"SELECT tgname, "
"tgfoid::pg_catalog.regproc AS tgfname, "
"tgtype, tgnargs, tgargs, tgenabled, "
"tgisconstraint, tgconstrname, tgdeferrable, "
"tgconstrrelid, tginitdeferred, tableoid, oid, "
"tgconstrrelid::pg_catalog.regclass AS
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Oct-20, Justin Pryzby wrote: > On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote: > > Alvaro Herrera writes: > > > Hmm, next question: should we backpatch a fix for this? (This applies > > > all the way back to 11.) If we do, then we would change behavior of > > > partition creation. It's hard to see that the current behavior is > > > desirable ... and I think anybody who would have come across this, would > > > wish it behaved the other way. But still -- it would definitely be a > > > behavior change. > > > > The behavior change seems like it'd be an improvement in a vacuum, > > but I wonder how it would interact with catalog contents left behind > > by the old misbehavior. Also, would we expect pg_dump to try to do > > anything to clean up the mess? If so, allowing a back branch to have > > had more than one behavior would complicate that greatly. > > I don't think there's a problem with catalog content ? > I think it's fine if there's an enabled child trigger inheriting from a > disabled parent? This changes the initial tgenabled for new partitions. I don't think we'd need to do anything special here ... particularly considering the discovery that pg_dump does not preserve the disable status of trigger on partitions: > However...it looks like pg_dump should ALTER the child trigger state if it > differ from its parent. Or maybe it needs to CREATE child triggers with the > proper state before attaching the child table ? I guess *something* needs to be done, but I'm not clear on what it is. Creating the trigger on partition beforehand does not work: an error is raised on attach that the trigger already exists. The only way I see to do this, is to have pg_dump extract tgenabled for all child triggers that doesn't have the same tgenabled as the parent, and append ALTER .. DISABLE commands for each one to the parent table trigger creation command. So pg_dump.c's getTriggers would have to obtain the list of altered child triggers, and then dumpTrigger would have to append the ALTER TABLE ONLY .. ENABLE/DISABLE command for that particular trigger.
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Oct-20, Alvaro Herrera wrote: > > diff --git a/src/backend/commands/tablecmds.c > > b/src/backend/commands/tablecmds.c > > index 511f015a86..c8d6f78da2 100644 > > --- a/src/backend/commands/tablecmds.c > > +++ b/src/backend/commands/tablecmds.c > > @@ -4321,6 +4321,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd > > *cmd, > > case AT_DisableTrigAll: > > case AT_DisableTrigUser: > > ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); > > + ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, > > context); > > pass = AT_PASS_MISC; > > break; > > case AT_EnableRule: /* ENABLE/DISABLE RULE variants > > */ > > I'll add tests for both cases and push to all branches 11+. Pushed this part.
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Tue, Oct 20, 2020 at 03:56:30PM -0400, Tom Lane wrote: > Alvaro Herrera writes: > > Hmm, next question: should we backpatch a fix for this? (This applies > > all the way back to 11.) If we do, then we would change behavior of > > partition creation. It's hard to see that the current behavior is > > desirable ... and I think anybody who would have come across this, would > > wish it behaved the other way. But still -- it would definitely be a > > behavior change. > > The behavior change seems like it'd be an improvement in a vacuum, > but I wonder how it would interact with catalog contents left behind > by the old misbehavior. Also, would we expect pg_dump to try to do > anything to clean up the mess? If so, allowing a back branch to have > had more than one behavior would complicate that greatly. I don't think there's a problem with catalog content ? I think it's fine if there's an enabled child trigger inheriting from a disabled parent? This changes the initial tgenabled for new partitions. The old catalog state is still possible - it's what you'd get if you did CREATE TABLE child PARTITION OF..; ALTER TABLE child DISABLE TRIGGER. I don't think pg_dump needs to do anyting special, since the restore will now do what's wanted without added commands. Note that pg_dump switched from "PARTITION OF" to "ATTACH PARTITION" at commit 33a53130a. This would handles both on the server side. However...it looks like pg_dump should ALTER the child trigger state if it differ from its parent. Or maybe it needs to CREATE child triggers with the proper state before attaching the child table ? -- Justin
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Oct-16, Alvaro Herrera wrote: > On 2020-Oct-16, Alvaro Herrera wrote: > > > I also just noticed that ALTER TABLE ONLY recurses to children, which it > > should not. > > Apparently I wrote (bogus) bespoke code to handle recursion in > EnableDisableTrigger instead of using ATSimpleRecursion. This patch > seems to fix this problem. ... but it affects legacy inheritance, which would be undesirable because it has never recursed for that case. So it needs to have a relkind check here and only recurse if it's a new-style partitioned table: > diff --git a/src/backend/commands/tablecmds.c > b/src/backend/commands/tablecmds.c > index 511f015a86..c8d6f78da2 100644 > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -4321,6 +4321,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd > *cmd, > case AT_DisableTrigAll: > case AT_DisableTrigUser: > ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); > + ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, > context); > pass = AT_PASS_MISC; > break; > case AT_EnableRule: /* ENABLE/DISABLE RULE variants > */ I'll add tests for both cases and push to all branches 11+.
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
Alvaro Herrera writes: > Hmm, next question: should we backpatch a fix for this? (This applies > all the way back to 11.) If we do, then we would change behavior of > partition creation. It's hard to see that the current behavior is > desirable ... and I think anybody who would have come across this, would > wish it behaved the other way. But still -- it would definitely be a > behavior change. The behavior change seems like it'd be an improvement in a vacuum, but I wonder how it would interact with catalog contents left behind by the old misbehavior. Also, would we expect pg_dump to try to do anything to clean up the mess? If so, allowing a back branch to have had more than one behavior would complicate that greatly. regards, tom lane
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On Tue, Oct 20, 2020 at 04:04:20PM -0300, Alvaro Herrera wrote:
> On 2020-Sep-30, Justin Pryzby wrote:
>
> > CREATE TABLE t(i int) PARTITION BY RANGE(i);
> > CREATE TABLE t1 PARTITION OF t 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 t FOR EACH ROW EXECUTE FUNCTION tgf();
> > ALTER TABLE t1 DISABLE TRIGGER tg;
> > INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> > ALTER TABLE t DISABLE TRIGGER tg;
> > CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
> >
> > postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE
> > tgrelid::regclass::text IN ('t1','t2');
> > tgrelid | tgenabled
> > -+---
> > t1 | D
> > t2 | O
> > (2 rows)
> >
> > I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> > (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> > the fix should be.
>
> Hmm, next question: should we backpatch a fix for this? (This applies
> all the way back to 11.) If we do, then we would change behavior of
> partition creation. It's hard to see that the current behavior is
> desirable ... and I think anybody who would have come across this, would
> wish it behaved the other way. But still -- it would definitely be a
> behavior change.
+0.8 to backpatch. To v13 if not further.
We don't normally disable triggers, otherwise I would say +1.
For context, I ran into this issue while migrating a customer to a new server
using pg_restore and a custom backup script which loops around pg_dump, and
handles partitioned tables differently depending if they're recent or historic.
Our backup job works well, but is technically a bit of a hack. It doesn't do
the right thing (causes sql errors and pg_restore warnings) for inherited
indexes and, apparently, triggers. Disabling the trigger was my 4th attempt to
handle an error restoring a specific table (mismatched column type between
parent dump and child dumped several days earlier). I eventually (5th
or 6th attempt) dropped the parent trigger, created the child tables using
--section=pre-data, ALTERed a column to match, and then ran post-data and
attached it.
--
Justin
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Sep-30, Justin Pryzby wrote:
> CREATE TABLE t(i int) PARTITION BY RANGE(i);
> CREATE TABLE t1 PARTITION OF t 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 t FOR EACH ROW EXECUTE FUNCTION tgf();
> ALTER TABLE t1 DISABLE TRIGGER tg;
> INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> ALTER TABLE t DISABLE TRIGGER tg;
> CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
>
> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE
> tgrelid::regclass::text IN ('t1','t2');
> tgrelid | tgenabled
> -+---
> t1 | D
> t2 | O
> (2 rows)
>
> I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.
Hmm, next question: should we backpatch a fix for this? (This applies
all the way back to 11.) If we do, then we would change behavior of
partition creation. It's hard to see that the current behavior is
desirable ... and I think anybody who would have come across this, would
wish it behaved the other way. But still -- it would definitely be a
behavior change.
This is a judgement call, and mine says to backpatch, but I've been
wrong on that.
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Oct-16, Alvaro Herrera wrote:
> I also just noticed that ALTER TABLE ONLY recurses to children, which it
> should not.
Apparently I wrote (bogus) bespoke code to handle recursion in
EnableDisableTrigger instead of using ATSimpleRecursion. This patch
seems to fix this problem.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 511f015a86..c8d6f78da2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4321,6 +4321,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrigAll:
case AT_DisableTrigUser:
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_MISC;
break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 3b4fbdadf4..b28b49bdfa 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1530,27 +1530,6 @@ EnableDisableTrigger(Relation rel, const char *tgname,
heap_freetuple(newtup);
- /*
- * When altering FOR EACH ROW triggers on a partitioned table, do
- * the same on the partitions as well.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
-(TRIGGER_FOR_ROW(oldtrig->tgtype)))
- {
-PartitionDesc partdesc = RelationGetPartitionDesc(rel);
-int i;
-
-for (i = 0; i < partdesc->nparts; i++)
-{
- Relation part;
-
- part = relation_open(partdesc->oids[i], lockmode);
- EnableDisableTrigger(part, NameStr(oldtrig->tgname),
- fires_when, skip_system, lockmode);
- table_close(part, NoLock); /* keep lock till commit */
-}
- }
-
changed = true;
}
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
Same, with a little test.
I also just noticed that ALTER TABLE ONLY recurses to children, which it
should not.
>From 2fb3a3122bdbbb1eb5aa6608b5132b8ab07096d4 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Fri, 16 Oct 2020 10:58:54 -0300
Subject: [PATCH] When cloning triggers, preserve enabling state
---
src/backend/commands/tablecmds.c | 8 +++---
src/backend/commands/trigger.c | 30
src/include/commands/trigger.h | 5
src/test/regress/expected/triggers.out | 39 ++
src/test/regress/sql/triggers.sql | 24
5 files changed, 96 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 511f015a86..f20b877f68 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16879,10 +16879,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
trigStmt->initdeferred = trigForm->tginitdeferred;
trigStmt->constrrel = NULL; /* passed separately */
- CreateTrigger(trigStmt, NULL, RelationGetRelid(partition),
- trigForm->tgconstrrelid, InvalidOid, InvalidOid,
- trigForm->tgfoid, trigForm->oid, qual,
- false, true);
+ CreateTriggerFiringOn(trigStmt, NULL, RelationGetRelid(partition),
+ trigForm->tgconstrrelid, InvalidOid, InvalidOid,
+ trigForm->tgfoid, trigForm->oid, qual,
+ false, true, trigForm->tgenabled);
MemoryContextSwitchTo(oldcxt);
MemoryContextReset(perTupCxt);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 3b4fbdadf4..b826b3595c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -158,6 +158,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;
@@ -800,7 +818,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);
@@ -1130,11 +1148,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 a40ddf5db5..40b8154876 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -162,6 +162,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 5e76b3a47e..d85471a3a9 100644
--- a/src/test/regress/expected/triggers.out
+++
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
On 2020-Sep-30, Justin Pryzby wrote:
> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE
> tgrelid::regclass::text IN ('t1','t2');
> tgrelid | tgenabled
> -+---
> t1 | D
> t2 | O
> (2 rows)
>
> I consider this a bug,
Yeah.
> but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.
I suggest we add a new function, as in the attached. It's important to
keep the ABI of CreateTrigger unchanged, for the sake of
backpatchability, but ISTM it's equally important to keep its API
unchanged, because outside callers such as ProcessUtility_slow does not
have to care about the new trigger's enabled state.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 511f015a86..f20b877f68 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16879,10 +16879,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
trigStmt->initdeferred = trigForm->tginitdeferred;
trigStmt->constrrel = NULL; /* passed separately */
- CreateTrigger(trigStmt, NULL, RelationGetRelid(partition),
- trigForm->tgconstrrelid, InvalidOid, InvalidOid,
- trigForm->tgfoid, trigForm->oid, qual,
- false, true);
+ CreateTriggerFiringOn(trigStmt, NULL, RelationGetRelid(partition),
+ trigForm->tgconstrrelid, InvalidOid, InvalidOid,
+ trigForm->tgfoid, trigForm->oid, qual,
+ false, true, trigForm->tgenabled);
MemoryContextSwitchTo(oldcxt);
MemoryContextReset(perTupCxt);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 3b4fbdadf4..b826b3595c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -158,6 +158,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;
@@ -800,7 +818,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);
@@ -1130,11 +1148,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 a40ddf5db5..40b8154876 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -162,6 +162,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);
Re: CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
I'm hoping that Alvaro will comment on this.
On Wed, Sep 30, 2020 at 05:34:50PM -0500, Justin Pryzby wrote:
> CREATE TABLE t(i int) PARTITION BY RANGE(i);
> CREATE TABLE t1 PARTITION OF t 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 t FOR EACH ROW EXECUTE FUNCTION tgf();
> ALTER TABLE t1 DISABLE TRIGGER tg;
> INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
> ALTER TABLE t DISABLE TRIGGER tg;
> CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
>
> postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE
> tgrelid::regclass::text IN ('t1','t2');
> tgrelid | tgenabled
> -+---
> t1 | D
> t2 | O
> (2 rows)
>
> I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
> (since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
> the fix should be.
CREATE TABLE .. PARTITION OF fails to preserve tgenabled for inherited row triggers
CREATE TABLE t(i int) PARTITION BY RANGE(i);
CREATE TABLE t1 PARTITION OF t 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 t FOR EACH ROW EXECUTE FUNCTION tgf();
ALTER TABLE t1 DISABLE TRIGGER tg;
INSERT INTO t VALUES(1); -- inserts when trigger is disabled: good
ALTER TABLE t DISABLE TRIGGER tg;
CREATE TABLE t2 PARTITION OF t FOR VALUES FROM (10) TO (20);
postgres=# SELECT tgrelid::regclass, tgenabled FROM pg_trigger WHERE
tgrelid::regclass::text IN ('t1','t2');
tgrelid | tgenabled
-+---
t1 | D
t2 | O
(2 rows)
I consider this a bug,but CreateTrigStmt doesn't have any "enabled" member
(since it's impossible to CREATE TRIGGER .. DISABLED), so I'm not sure where
the fix should be.
