Trigger with WHEN clause (WIP)

I'm working on WHEN clause support in triggers.
I heard that the feature is in the SQL standard.

We can rewrite triggers that uses suppress_redundant_updates_trigger
http://www.postgresql.org/docs/8.4/static/functions-trigger.html
with WHEN clause:
CREATE TRIGGER modified_any
BEFORE UPDATE ON tbl
FOR EACH ROW
WHEN (OLD.* IS DISTINCT FROM NEW.*)
EXECUTE PROCEDURE trigger_func();

I think there is a benefit to provide WHEN cluase at least
for compatibility with other DBMSs, even through we can move
the expressions into the body of trigger functions.

WIP-patch attached. It adds 'tgqual' text field into pg_trigger.
It works at first glance, but surely needs some adjustments
especially in the usage of TupleTableSlot in TriggerEnabled().

I have a question about executing qualification expression.
I used ecxt_innertuple for old tuples and ecxt_outertuple
for new tuples, but I'm not sure it is the right way.
Comments and suggestions welcome.

James Pye <lists(at)jwp(dot)name> wrote:
> Well, looks like WHEN is, or is going to be standard:
>
> <triggered action> ::=
> [ FOREACH { ROW | STATEMENT } ]
> [ WHEN<left paren><search condition> <right paren> ]
> <triggered SQL statement>
>
> (page 653 from 5CD2-02-Foundation-2006-01)

David Fetter <david(at)fetter(dot)org> wrote:
> Page 674 of 6WD_02_Foundation_2007-12 has a similar thing:
>
> <triggered action> ::=
> [ FOR EACH { ROW | STATEMENT } ]
> [ <triggered when clause> ]
> <triggered SQL statement>
>
> <triggered when clause> ::=
> WHEN <left paren> <search condition> <right paren>

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> I think there is a benefit to provide WHEN cluase at least
> for compatibility with other DBMSs, even through we can move
> the expressions into the body of trigger functions.

This seems to me to be a lot of code to accomplish nothing useful.
It will always be the case that any nontrivial logic has to be done
inside the trigger. And the compatibility argument is entirely
pointless given the lack of compatibility in the trigger function
itself.

-1

regards, tom lane

2009/10/15 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>> I think there is a benefit to provide WHEN cluase at least
>> for compatibility with other DBMSs, even through we can move
>> the expressions into the body of trigger functions.
>
> This seems to me to be a lot of code to accomplish nothing useful.
> It will always be the case that any nontrivial logic has to be done
> inside the trigger.  And the compatibility argument is entirely
> pointless given the lack of compatibility in the trigger function
> itself.
>
> -1
>

I disagree. When I analysed speed of some operations, I found some
unwanted trigger calls should to slow down applications. I am for any
method, that could to decrease trigger calls.

Regards
Pavel Stehule

>                        regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2009/10/15 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> This seems to me to be a lot of code to accomplish nothing useful.

> I disagree. When I analysed speed of some operations, I found some
> unwanted trigger calls should to slow down applications. I am for any
> method, that could to decrease trigger calls.

That argument is based on a completely evidence-free assumption, namely
that this patch would make your case faster. Executing the WHEN tests
is hardly going to be zero cost. It's not too hard to postulate cases
where implementing a filter this way would be *slower* than doing it
inside the trigger.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> That argument is based on a completely evidence-free assumption, namely
> that this patch would make your case faster. Executing the WHEN tests
> is hardly going to be zero cost. It's not too hard to postulate cases
> where implementing a filter this way would be *slower* than doing it
> inside the trigger.

It's pretty often the case (IME) that calling a trigger is the only
point in the session where you fire plpgsql, and that's a visible
cost. Last time I had to measure it, it was 1ms per call. We were trying
to optimize queries running in 3ms to 4ms, called more than 100 times a
second (in parallel on multi core architecture, but still).

The way I understand it, having the WHEN clause in CREATE TRIGGER would
allow to filter out some interpreter initialisations.

Regards,
--
dim

Dimitri Fontaine <dfontaine(at)hi-media(dot)com> wrote:

> It's pretty often the case (IME) that calling a trigger is the only
> point in the session where you fire plpgsql, and that's a visible
> cost.

Wouldn't a connection pool solve this?

-Kevin

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > I think there is a benefit to provide WHEN cluase at least
> > for compatibility with other DBMSs, even through we can move
> > the expressions into the body of trigger functions.
>
> This seems to me to be a lot of code to accomplish nothing useful.
> It will always be the case that any nontrivial logic has to be done
> inside the trigger. And the compatibility argument is entirely
> pointless given the lack of compatibility in the trigger function
> itself.

I see that WHEN cluase is not always needed,
but I think there are several benefits to have it:

* WHEN cluase is in SQL standard.

* We could recheck trigger conditions when NEW tuple is modified.
(not yet implemented in the patch, though)
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00286.php

* As for compatibility, it is easy for typical users to move trigger
bodies into a function, but they don't like to merge the condition
and the bodies into a function in my experience.

* As for performance, I don't have any evidence. But there was a
discussion about benefits of writing partitioning trigger function
with C instead of PL/pgSQL. He said pgplsql is slow.
http://archives.postgresql.org/pgsql-hackers/2008-12/msg01221.php
Also, to separate trigger bodies and conditions are useful when we
write triggers in C because we don't have to recomplie the code.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Thu, 2009-10-15 at 16:54 -0500, Kevin Grittner wrote:
> Dimitri Fontaine <dfontaine(at)hi-media(dot)com> wrote:
>
> > It's pretty often the case (IME) that calling a trigger is the only
> > point in the session where you fire plpgsql, and that's a visible
> > cost.
>
> Wouldn't a connection pool solve this?

No

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-10-16 at 10:14 +0900, Itagaki Takahiro wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> > > I think there is a benefit to provide WHEN cluase at least
> > > for compatibility with other DBMSs, even through we can move
> > > the expressions into the body of trigger functions.
> >
> > This seems to me to be a lot of code to accomplish nothing useful.
> > It will always be the case that any nontrivial logic has to be done
> > inside the trigger. And the compatibility argument is entirely
> > pointless given the lack of compatibility in the trigger function
> > itself.
>
> I see that WHEN cluase is not always needed,
> but I think there are several benefits to have it:
>
> * WHEN cluase is in SQL standard.
>
> * We could recheck trigger conditions when NEW tuple is modified.
> (not yet implemented in the patch, though)
> http://archives.postgresql.org/pgsql-hackers/2009-09/msg00286.php
>
> * As for compatibility, it is easy for typical users to move trigger
> bodies into a function, but they don't like to merge the condition
> and the bodies into a function in my experience.

+1 because

* It is SQL Standard
* Other RDBMS support it (Oracle, DB2)
* It will reduce size of in-memory pending trigger list (for large
statements) and avoid invoking run-time of function handlers, important
for heavier PLs (for small statements)

I also support addition of INSTEAD OF triggers for first two reasons and
because it may be an easier route to allow updateable views.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2009-10-16 at 09:31 +0100, Simon Riggs wrote:
> * It will reduce size of in-memory pending trigger list (for large
> statements)

But this won't be the outcome when it's implemented the way it is being
proposed, which checks the where clause just before executing the
trigger function.

On Fri, 2009-10-16 at 14:39 +0300, Peter Eisentraut wrote:
> On Fri, 2009-10-16 at 09:31 +0100, Simon Riggs wrote:
> > * It will reduce size of in-memory pending trigger list (for large
> > statements)
>
> But this won't be the outcome when it's implemented the way it is being
> proposed, which checks the where clause just before executing the
> trigger function.

Thanks for pointing that out.

I'm giving reasons why we'd want a WHEN clause. If the current
implementation doesn't do all that it could, then ISTM thats a reason to
reject patch for now, but not for all time.

Incidentally, re-accessing a data block at end of statement may have
caused to block to fall out of cache and then be re-accessed again. So
optimising that away can save on I/O as well.

--
Simon Riggs www.2ndQuadrant.com

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On Fri, 2009-10-16 at 09:31 +0100, Simon Riggs wrote:
>> * It will reduce size of in-memory pending trigger list (for large
>> statements)

> But this won't be the outcome when it's implemented the way it is being
> proposed, which checks the where clause just before executing the
> trigger function.

Hm, the feature could actually be worth something if it allows
conditions to be checked before an AFTER trigger event gets pushed
into the event list. However, if it's not doing that ...

I note BTW that we have some ad-hoc logic already that arranges to
suppress queuing of AFTER events for FK triggers, if the FK column
value has not changed. It might be interesting to look at whether
that hack could be unified with the user-accessible feature. It's
essentially a WHEN OLD.x IS NOT DISTINCT FROM NEW.x test.

regards, tom lane

On Fri, 2009-10-16 at 10:02 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On Fri, 2009-10-16 at 09:31 +0100, Simon Riggs wrote:
> >> * It will reduce size of in-memory pending trigger list (for large
> >> statements)
>
> > But this won't be the outcome when it's implemented the way it is being
> > proposed, which checks the where clause just before executing the
> > trigger function.
>
> Hm, the feature could actually be worth something if it allows
> conditions to be checked before an AFTER trigger event gets pushed
> into the event list. However, if it's not doing that ...
>
> I note BTW that we have some ad-hoc logic already that arranges to
> suppress queuing of AFTER events for FK triggers, if the FK column
> value has not changed. It might be interesting to look at whether
> that hack could be unified with the user-accessible feature. It's
> essentially a WHEN OLD.x IS NOT DISTINCT FROM NEW.x test.

If the function is idempotent then we can also optimise away multiple
calls by checking whether a similar call is already queued.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> If the function is idempotent then we can also optimise away multiple
> calls by checking whether a similar call is already queued.

But how would we know that? It seems orthogonal to this patch,
anyway.

regards, tom lane

On Fri, 16 Oct 2009, Tom Lane wrote:

> I note BTW that we have some ad-hoc logic already that arranges to
> suppress queuing of AFTER events for FK triggers, if the FK column
> value has not changed. It might be interesting to look at whether
> that hack could be unified with the user-accessible feature. It's
> essentially a WHEN OLD.x IS NOT DISTINCT FROM NEW.x test.

Note that one of those cases (RI_TRIGGER_FK) is a bit special due to the
transaction id test. It might be worth seeing if a better solution is
possible to cover the case in the comment if the above becomes possible,
though.

Stephan Szabo <sszabo(at)megazone(dot)bigpanda(dot)com> writes:
> On Fri, 16 Oct 2009, Tom Lane wrote:
>> I note BTW that we have some ad-hoc logic already that arranges to
>> suppress queuing of AFTER events for FK triggers, if the FK column
>> value has not changed. It might be interesting to look at whether
>> that hack could be unified with the user-accessible feature. It's
>> essentially a WHEN OLD.x IS NOT DISTINCT FROM NEW.x test.

> Note that one of those cases (RI_TRIGGER_FK) is a bit special due to the
> transaction id test. It might be worth seeing if a better solution is
> possible to cover the case in the comment if the above becomes possible,
> though.

The existing FK short-circuit code is presumably faster than generic
WHEN expression evaluation would be, so I wasn't really imagining that
we would take it out in favor of using the generic feature. But
possibly we could clean things up to some extent by treating the FK case
as a variant of generic WHEN. One particular thing I never much liked
about that hack is its dependence on specific function OIDs. I could
see replacing that with some representation that says "this trigger
has a special FK WHEN clause" as opposed to providing an expression
tree.

regards, tom lane