Allow WHEN in INSTEAD OF triggers
Folks,
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.
I noticed that columns were also disallowed in INSTEAD OF triggers,
but haven't dug further into those just yet.
What say?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v1-0001-Allow-WHEN-in-INSTEAD-OF-triggers.patchtext/x-diff; charset=us-asciiDownload
From cb0bb4b39efff33d7eee7cd4cde7879b7107d250 Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Fri, 27 Dec 2019 18:57:31 -0800
Subject: [PATCH v1] Allow WHEN in INSTEAD OF triggers
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"
This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
This was disallowed for reasons that aren't entirely obvious, so allow.
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index 3339a4b4e1..b822cb6e8d 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -377,10 +377,6 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</
and <literal>DELETE</literal> triggers cannot refer to <literal>NEW</literal>.
</para>
- <para><literal>INSTEAD OF</literal> triggers do not support <literal>WHEN</literal>
- conditions.
- </para>
-
<para>
Currently, <literal>WHEN</literal> expressions cannot contain
subqueries.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 36093a29a8..c4cc07a426 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -381,17 +381,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("TRUNCATE FOR EACH ROW triggers are not supported")));
- /* INSTEAD triggers must be row-level, and can't have WHEN or columns */
+ /* INSTEAD triggers must be row-level, and can't have columns */
if (TRIGGER_FOR_INSTEAD(tgtype))
{
if (!TRIGGER_FOR_ROW(tgtype))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("INSTEAD OF triggers must be FOR EACH ROW")));
- if (stmt->whenClause)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("INSTEAD OF triggers cannot have WHEN conditions")));
if (stmt->columns != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 1e4053ceed..ee2b63cb03 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -1030,10 +1030,6 @@ CREATE TRIGGER invalid_trig INSTEAD OF DELETE ON main_table
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
ERROR: "main_table" is a table
DETAIL: Tables cannot have INSTEAD OF triggers.
--- Don't support WHEN clauses with INSTEAD OF triggers
-CREATE TRIGGER invalid_trig INSTEAD OF UPDATE ON main_view
-FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE view_trigger('instead_of_upd');
-ERROR: INSTEAD OF triggers cannot have WHEN conditions
-- Don't support column-level INSTEAD OF triggers
CREATE TRIGGER invalid_trig INSTEAD OF UPDATE OF a ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index c21b6c124e..840d6617da 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -727,10 +727,6 @@ FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
CREATE TRIGGER invalid_trig INSTEAD OF DELETE ON main_table
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
--- Don't support WHEN clauses with INSTEAD OF triggers
-CREATE TRIGGER invalid_trig INSTEAD OF UPDATE ON main_view
-FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE view_trigger('instead_of_upd');
-
-- Don't support column-level INSTEAD OF triggers
CREATE TRIGGER invalid_trig INSTEAD OF UPDATE OF a ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
--------------2.24.1--
On 2019-Dec-28, David Fetter wrote:
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.
If you want to remove the restriction, your patch should add some test
cases that show it working.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
David Fetter <david@fetter.org> writes:
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.
This seems like a remarkably bad idea. The point of an INSTEAD OF
trigger is that it is guaranteed to handle the operation. What's
the system supposed to do with rows the trigger doesn't handle?
I notice that your patch doesn't even bother to test what happens,
but I'd argue that whatever it is, it's wrong. If you think that
"do nothing" or "throw an error" is appropriate, you can code that
inside the trigger. It's not PG's charter to make such a decision.
regards, tom lane
PS: I think your chances of removing rules are not good, either.
On Fri, Dec 27, 2019 at 10:29:15PM -0500, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.This seems like a remarkably bad idea. The point of an INSTEAD OF
trigger is that it is guaranteed to handle the operation. What's
the system supposed to do with rows the trigger doesn't handle?
Nothing. Why would it be different from the other forms of WHEN in
triggers?
I notice that your patch doesn't even bother to test what happens,
but I'd argue that whatever it is, it's wrong. If you think that
"do nothing" or "throw an error" is appropriate, you can code that
inside the trigger. It's not PG's charter to make such a decision.
If that's the case, why do we have WHEN for triggers at all? I'm just
working toward make them more consistent. From a UX perspective, it's
a lot simpler and clearer to do this in the trigger declaration than
it is in the body.
PS: I think your chances of removing rules are not good, either.
I suspect I have a lot of company in my view of user-modifiable
rewrite rules as an experiment we can finally discontinue in view of
its decisive results.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Sat, Dec 28, 2019 at 12:12:30AM -0300, Alvaro Herrera wrote:
On 2019-Dec-28, David Fetter wrote:
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.If you want to remove the restriction, your patch should add some test
cases that show it working.
Tests added :)
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v2-0001-Allow-WHEN-in-INSTEAD-OF-triggers.patchtext/x-diff; charset=us-asciiDownload
From d6af8b66347b31e14d961e83023dbcb658bea64b Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Fri, 27 Dec 2019 18:57:31 -0800
Subject: [PATCH v2] Allow WHEN in INSTEAD OF triggers
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.24.1"
This is a multi-part message in MIME format.
--------------2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
This was disallowed for reasons that aren't entirely obvious, so allow.
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index 3339a4b4e1..b822cb6e8d 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -377,10 +377,6 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</
and <literal>DELETE</literal> triggers cannot refer to <literal>NEW</literal>.
</para>
- <para><literal>INSTEAD OF</literal> triggers do not support <literal>WHEN</literal>
- conditions.
- </para>
-
<para>
Currently, <literal>WHEN</literal> expressions cannot contain
subqueries.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 36093a29a8..c4cc07a426 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -381,17 +381,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("TRUNCATE FOR EACH ROW triggers are not supported")));
- /* INSTEAD triggers must be row-level, and can't have WHEN or columns */
+ /* INSTEAD triggers must be row-level, and can't have columns */
if (TRIGGER_FOR_INSTEAD(tgtype))
{
if (!TRIGGER_FOR_ROW(tgtype))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("INSTEAD OF triggers must be FOR EACH ROW")));
- if (stmt->whenClause)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("INSTEAD OF triggers cannot have WHEN conditions")));
if (stmt->columns != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 1e4053ceed..f44f28760e 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -965,7 +965,11 @@ begin
end if;
if TG_OP = 'UPDATE' then
- raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+ if strpos(argstr, 'instead_of_when') > 0 then
+ raise NOTICE 'instead_of_when fired';
+ else
+ raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+ end if;
UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = OLD.a AND b = OLD.b;
if NOT FOUND then RETURN NULL; end if;
RETURN NEW;
@@ -1030,10 +1034,6 @@ CREATE TRIGGER invalid_trig INSTEAD OF DELETE ON main_table
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
ERROR: "main_table" is a table
DETAIL: Tables cannot have INSTEAD OF triggers.
--- Don't support WHEN clauses with INSTEAD OF triggers
-CREATE TRIGGER invalid_trig INSTEAD OF UPDATE ON main_view
-FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE view_trigger('instead_of_upd');
-ERROR: INSTEAD OF triggers cannot have WHEN conditions
-- Don't support column-level INSTEAD OF triggers
CREATE TRIGGER invalid_trig INSTEAD OF UPDATE OF a ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
@@ -1049,6 +1049,9 @@ CREATE TRIGGER instead_of_update_trig INSTEAD OF UPDATE ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
CREATE TRIGGER instead_of_delete_trig INSTEAD OF DELETE ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
+CREATE TRIGGER when_different_update INSTEAD OF UPDATE ON main_view
+FOR EACH ROW WHEN (OLD.a IS DISTINCT FROM NEW.a)
+EXECUTE PROCEDURE view_trigger('instead_of_when');
-- Valid BEFORE statement VIEW triggers
CREATE TRIGGER before_ins_stmt_trig BEFORE INSERT ON main_view
FOR EACH STATEMENT EXECUTE PROCEDURE view_trigger('before_view_ins_stmt');
@@ -1145,18 +1148,47 @@ UPDATE 1
UPDATE main_view SET b = 0 WHERE false;
NOTICE: main_view BEFORE UPDATE STATEMENT (before_view_upd_stmt)
NOTICE: main_view AFTER UPDATE STATEMENT (after_view_upd_stmt)
+UPDATE 0
+-- INSTEAD OF ... WHEN trigger fires.
+UPDATE main_view SET a = 23 WHERE a = 21 RETURNING *;
+NOTICE: main_view BEFORE UPDATE STATEMENT (before_view_upd_stmt)
+NOTICE: main_view INSTEAD OF UPDATE ROW (instead_of_upd)
+NOTICE: OLD: (21,10), NEW: (23,10)
+NOTICE: trigger_func(before_upd_a_stmt) called: action = UPDATE, when = BEFORE, level = STATEMENT
+NOTICE: trigger_func(after_upd_a_b_row) called: action = UPDATE, when = AFTER, level = ROW
+NOTICE: trigger_func(after_upd_b_row) called: action = UPDATE, when = AFTER, level = ROW
+NOTICE: trigger_func(after_upd_b_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: trigger_func(after_upd_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: main_view INSTEAD OF UPDATE ROW (instead_of_when)
+NOTICE: instead_of_when fired
+NOTICE: trigger_func(before_upd_a_stmt) called: action = UPDATE, when = BEFORE, level = STATEMENT
+NOTICE: trigger_func(after_upd_b_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: trigger_func(after_upd_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: main_view INSTEAD OF UPDATE ROW (instead_of_upd)
+NOTICE: OLD: (21,32), NEW: (23,32)
+NOTICE: trigger_func(before_upd_a_stmt) called: action = UPDATE, when = BEFORE, level = STATEMENT
+NOTICE: trigger_func(after_upd_a_b_row) called: action = UPDATE, when = AFTER, level = ROW
+NOTICE: trigger_func(after_upd_b_row) called: action = UPDATE, when = AFTER, level = ROW
+NOTICE: trigger_func(after_upd_b_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: trigger_func(after_upd_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: main_view INSTEAD OF UPDATE ROW (instead_of_when)
+NOTICE: instead_of_when fired
+NOTICE: trigger_func(before_upd_a_stmt) called: action = UPDATE, when = BEFORE, level = STATEMENT
+NOTICE: trigger_func(after_upd_b_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: trigger_func(after_upd_stmt) called: action = UPDATE, when = AFTER, level = STATEMENT
+NOTICE: main_view AFTER UPDATE STATEMENT (after_view_upd_stmt)
+ a | b
+---+---
+(0 rows)
+
UPDATE 0
-- Delete from view using trigger
DELETE FROM main_view WHERE a IN (20,21);
NOTICE: main_view BEFORE DELETE STATEMENT (before_view_del_stmt)
NOTICE: main_view INSTEAD OF DELETE ROW (instead_of_del)
-NOTICE: OLD: (21,10)
-NOTICE: main_view INSTEAD OF DELETE ROW (instead_of_del)
NOTICE: OLD: (20,31)
-NOTICE: main_view INSTEAD OF DELETE ROW (instead_of_del)
-NOTICE: OLD: (21,32)
NOTICE: main_view AFTER DELETE STATEMENT (after_view_del_stmt)
-DELETE 3
+DELETE 1
DELETE FROM main_view WHERE a = 31 RETURNING a, b;
NOTICE: main_view BEFORE DELETE STATEMENT (before_view_del_stmt)
NOTICE: main_view INSTEAD OF DELETE ROW (instead_of_del)
@@ -1186,6 +1218,7 @@ Triggers:
instead_of_delete_trig INSTEAD OF DELETE ON main_view FOR EACH ROW EXECUTE FUNCTION view_trigger('instead_of_del')
instead_of_insert_trig INSTEAD OF INSERT ON main_view FOR EACH ROW EXECUTE FUNCTION view_trigger('instead_of_ins')
instead_of_update_trig INSTEAD OF UPDATE ON main_view FOR EACH ROW EXECUTE FUNCTION view_trigger('instead_of_upd')
+ when_different_update INSTEAD OF UPDATE ON main_view FOR EACH ROW WHEN (old.a IS DISTINCT FROM new.a) EXECUTE FUNCTION view_trigger('instead_of_when')
-- Test dropping view triggers
DROP TRIGGER instead_of_insert_trig ON main_view;
@@ -1208,6 +1241,7 @@ Triggers:
before_ins_stmt_trig BEFORE INSERT ON main_view FOR EACH STATEMENT EXECUTE FUNCTION view_trigger('before_view_ins_stmt')
before_upd_stmt_trig BEFORE UPDATE ON main_view FOR EACH STATEMENT EXECUTE FUNCTION view_trigger('before_view_upd_stmt')
instead_of_update_trig INSTEAD OF UPDATE ON main_view FOR EACH ROW EXECUTE FUNCTION view_trigger('instead_of_upd')
+ when_different_update INSTEAD OF UPDATE ON main_view FOR EACH ROW WHEN (old.a IS DISTINCT FROM new.a) EXECUTE FUNCTION view_trigger('instead_of_when')
DROP VIEW main_view;
--
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index c21b6c124e..2e2583b0bc 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -672,7 +672,11 @@ begin
end if;
if TG_OP = 'UPDATE' then
- raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+ if strpos(argstr, 'instead_of_when') > 0 then
+ raise NOTICE 'instead_of_when fired';
+ else
+ raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+ end if;
UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = OLD.a AND b = OLD.b;
if NOT FOUND then RETURN NULL; end if;
RETURN NEW;
@@ -727,10 +731,6 @@ FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
CREATE TRIGGER invalid_trig INSTEAD OF DELETE ON main_table
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
--- Don't support WHEN clauses with INSTEAD OF triggers
-CREATE TRIGGER invalid_trig INSTEAD OF UPDATE ON main_view
-FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE view_trigger('instead_of_upd');
-
-- Don't support column-level INSTEAD OF triggers
CREATE TRIGGER invalid_trig INSTEAD OF UPDATE OF a ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
@@ -749,6 +749,10 @@ FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
CREATE TRIGGER instead_of_delete_trig INSTEAD OF DELETE ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
+CREATE TRIGGER when_different_update INSTEAD OF UPDATE ON main_view
+FOR EACH ROW WHEN (OLD.a IS DISTINCT FROM NEW.a)
+EXECUTE PROCEDURE view_trigger('instead_of_when');
+
-- Valid BEFORE statement VIEW triggers
CREATE TRIGGER before_ins_stmt_trig BEFORE INSERT ON main_view
FOR EACH STATEMENT EXECUTE PROCEDURE view_trigger('before_view_ins_stmt');
@@ -787,6 +791,9 @@ UPDATE main_view SET b = 32 WHERE a = 21 AND b = 31 RETURNING a, b;
-- Before and after stmt triggers should fire even when no rows are affected
UPDATE main_view SET b = 0 WHERE false;
+-- INSTEAD OF ... WHEN trigger fires.
+UPDATE main_view SET a = 23 WHERE a = 21 RETURNING *;
+
-- Delete from view using trigger
DELETE FROM main_view WHERE a IN (20,21);
DELETE FROM main_view WHERE a = 31 RETURNING a, b;
--------------2.24.1--
On Sat, 28 Dec 2019 at 16:45, David Fetter <david@fetter.org> wrote:
On Sat, Dec 28, 2019 at 12:12:30AM -0300, Alvaro Herrera wrote:
On 2019-Dec-28, David Fetter wrote:
While noodling around with an upcoming patch to remove user-modifiable
RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
triggers for no discernible reason. This patch removes that
restriction.If you want to remove the restriction, your patch should add some test
cases that show it working.Tests added :)
I too think this is a bad idea.
Doing nothing if the trigger's WHEN condition isn't satisfied is not
consistent with the way other types of trigger work -- with any other
type of trigger, if the WHEN condition doesn't evaluate to true, the
query goes ahead as if the trigger hadn't been there. So the most
consistent thing to do would be to attempt an auto-update if the
trigger isn't fired, and that leads to a whole other world of pain
(e.g., you'd need 2 completely different query plans for the 2 cases,
and more if you had views on top of other views).
The SQL spec explicitly states that INSTEAD OF triggers on views
should not have WHEN clauses, and for good reason. There are cases
where it makes sense to deviate from the spec, but I don't think this
is one of them.
Regards,
Dean