Re: Allow WHEN in INSTEAD OF triggers

2020-01-06 Thread Dean Rasheed 
On Sat, 28 Dec 2019 at 16:45, David Fetter  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

Re: Allow WHEN in INSTEAD OF triggers

2019-12-28 Thread David Fetter 
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  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From d6af8b66347b31e14d961e83023dbcb658bea64b Mon Sep 17 00:00:00 2001
From: David Fetter 
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 column_name1 [, column_name2DELETE triggers cannot refer to NEW.
  
 
- INSTEAD OF triggers do not support WHEN
-  conditions.
- 
-
  
   Currently, WHEN 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

Re: Allow WHEN in INSTEAD OF triggers

2019-12-28 Thread David Fetter 
On Fri, Dec 27, 2019 at 10:29:15PM -0500, Tom Lane wrote:
> David Fetter  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  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Allow WHEN in INSTEAD OF triggers

2019-12-27 Thread Tom Lane 
David Fetter  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.

Re: Allow WHEN in INSTEAD OF triggers

2019-12-27 Thread Alvaro Herrera 
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Allow WHEN in INSTEAD OF triggers

2019-12-27 Thread David Fetter 
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  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From cb0bb4b39efff33d7eee7cd4cde7879b7107d250 Mon Sep 17 00:00:00 2001
From: David Fetter 
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 column_name1 [, column_name2DELETE triggers cannot refer to NEW.
  
 
- INSTEAD OF triggers do not support WHEN
-  conditions.
- 
-
  
   Currently, WHEN 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--