Обсуждение: Triggers on columns

Here is a patch to implement "Support triggers on columns" in our ToDo list.

The syntax is:
    CREATE TRIGGER name
        BEFORE UPDATE OF col1, col12, ...
        ON tbl FOR EACH ROW EXECUTE PROCEDURE func();

I consulted the previous work following:
    Column-level triggers (From: Greg Sabino Mullane, Date: 2005-07-04)
    http://archives.postgresql.org/pgsql-patches/2005-07/msg00107.php
and completed some under-construction parts.

It's still arguable that we should add dependencies from column
triggers to referenced columns. In the present patch, dropeed
columns are just ignored and always considered as not-modified.
Please grep with "TODO: (TRIGGER)" to check the issue.

Comments welcome.

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

On Thu, Sep 03, 2009 at 10:52:09AM +0900, Itagaki Takahiro wrote:
> Here is a patch to implement "Support triggers on columns" in our ToDo list.
> 
> The syntax is:
>     CREATE TRIGGER name
>         BEFORE UPDATE OF col1, col12, ...
>         ON tbl FOR EACH ROW EXECUTE PROCEDURE func();

Kudos!

> 
> I consulted the previous work following:
>     Column-level triggers (From: Greg Sabino Mullane, Date: 2005-07-04)
>     http://archives.postgresql.org/pgsql-patches/2005-07/msg00107.php
> and completed some under-construction parts.
> 
> It's still arguable that we should add dependencies from column
> triggers to referenced columns.

+1 for adding the dependencies.

Cheers,
David.
> In the present patch, dropeed
> columns are just ignored and always considered as not-modified.
> Please grep with "TODO: (TRIGGER)" to check the issue.
> 
> Comments welcome.
> 
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center


> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

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

On Wed, Sep 2, 2009 at 9:52 PM, Itagaki
Takahiro<itagaki.takahiro@oss.ntt.co.jp> wrote:
> Here is a patch to implement "Support triggers on columns" in our ToDo list.
>
> The syntax is:
>    CREATE TRIGGER name
>        BEFORE UPDATE OF col1, col12, ...
>        ON tbl FOR EACH ROW EXECUTE PROCEDURE func();
>
> I consulted the previous work following:
>    Column-level triggers (From: Greg Sabino Mullane, Date: 2005-07-04)
>    http://archives.postgresql.org/pgsql-patches/2005-07/msg00107.php
> and completed some under-construction parts.
>
> It's still arguable that we should add dependencies from column
> triggers to referenced columns. In the present patch, dropeed
> columns are just ignored and always considered as not-modified.
> Please grep with "TODO: (TRIGGER)" to check the issue.
>
> Comments welcome.

Wow, so I wouldn't have to do this any more?

IF (TG_OP = 'UPDATE') THEN   IF (OLD.foo IS NOT DISTINCT FROM NEW.foo AND OLD.bar IS NOT
DISTINCT FROM NEW.bar        AND OLD.baz IS NOT DISTINCT FROM NEW.baz) THEN       RETURN NULL;   END IF;
END IF;

Apart from any possible gain in efficiency, the sheer savings in
typing sound quite awesome.

...Robert

Robert Haas <robertmhaas@gmail.com> wrote:

> Wow, so I wouldn't have to do this any more?
> 
> IF (TG_OP = 'UPDATE') THEN
>     IF (OLD.foo IS NOT DISTINCT FROM NEW.foo AND OLD.bar IS NOT
> DISTINCT FROM NEW.bar
>          AND OLD.baz IS NOT DISTINCT FROM NEW.baz) THEN
>         RETURN NULL;
>     END IF;
> END IF;

Sure, and I found there might be difference between "UPDATE" and
"UPDATE OF {all-columns}" triggers. UPDATE trigger is always fired
when a row is updated even if none of the columns are actually
modified, but UPDATE OF {all-columns} trigger is fired only when
at least one of the columns is modified.

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

David Fetter <david@fetter.org> wrote:

> > It's still arguable that we should add dependencies from column
> > triggers to referenced columns.
> 
> +1 for adding the dependencies.

But how? First, I tried to use existing dependency mechanism:
   ObjectAddress referenced;   referenced.classId = AttributeRelationId;   referenced.objectId = {relid};
referenced.objectSubId= {attnum};   recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
 

but we don't use ObjectAddress with classId = AttributeRelationId
for now in any places. Does it work? or do I also need to modify
dependency.c to support dependency-to-columns?

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

Itagaki Takahiro wrote:
> 
> David Fetter <david@fetter.org> wrote:
> 
> > > It's still arguable that we should add dependencies from column
> > > triggers to referenced columns.
> > 
> > +1 for adding the dependencies.
> 
> But how? First, I tried to use existing dependency mechanism:
> 
>     ObjectAddress referenced;
>     referenced.classId = AttributeRelationId;
>     referenced.objectId = {relid};
>     referenced.objectSubId = {attnum};
>     recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
> 
> but we don't use ObjectAddress with classId = AttributeRelationId
> for now in any places. Does it work?

Well, apparently you've been tasked with making sure it works properly :-)
The only problem I see with it is the fact that the objectId is not the
attribute's OID, but it should be possible.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> But how? First, I tried to use existing dependency mechanism:

>     ObjectAddress referenced;
>     referenced.classId = AttributeRelationId;
>     referenced.objectId = {relid};
>     referenced.objectSubId = {attnum};

This is just wrong.  The correct representation of a column isclassId = RelationRelationIdobjectId = relidobjectSubId =
attnum
The column is a sub-object of a pg_class item, not an object in
its own right.
        regards, tom lane

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Sure, and I found there might be difference between "UPDATE" and
> "UPDATE OF {all-columns}" triggers. UPDATE trigger is always fired
> when a row is updated even if none of the columns are actually
> modified, but UPDATE OF {all-columns} trigger is fired only when
> at least one of the columns is modified.

I'm betraying the fact that I haven't read the patch, but ...
exactly how, and when, are you determining whether a column has
been "modified"?  I can't count the number of times somebody
has proposed simplistic and incorrect solutions to that.
Usually they forget about BEFORE triggers changing the row.
        regards, tom lane

Tom Lane wrote:
> Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
>> Sure, and I found there might be difference between "UPDATE" and
>> "UPDATE OF {all-columns}" triggers. UPDATE trigger is always fired
>> when a row is updated even if none of the columns are actually
>> modified, but UPDATE OF {all-columns} trigger is fired only when
>> at least one of the columns is modified.
> 
> I'm betraying the fact that I haven't read the patch, but ...
> exactly how, and when, are you determining whether a column has
> been "modified"?  I can't count the number of times somebody
> has proposed simplistic and incorrect solutions to that.
> Usually they forget about BEFORE triggers changing the row.

It uses heap_tuple_attr_equals() to check whether a certain
column is modified, or not.

Itagaki-san, isn't it more suitable to check rte->modifiedCols
than heap_tuple_attr_equals()? Although, this information is
not delivered to executor...

What is the correct behavior when UPDATE statement set a new
value but it was identical to the original value?
In this case, heap_tuple_attr_equals() cannot detect the column
is used as a target of the UPDATE.

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> exactly how, and when, are you determining whether a column has
> been "modified"?  I can't count the number of times somebody
> has proposed simplistic and incorrect solutions to that.
> Usually they forget about BEFORE triggers changing the row.

There are some approaches:
1. Just check conditions in alphabetical order. Ignore subsequent   modifications after the conditions are examined.
2. Recheck conditions if NEW values are modified, but triggers that   have been fired already are not executed twice.
3. Column triggers are called after non-conditional UPDATE triggers   and column triggers cannot modify NEW values.

I like approach 2. because it is the most user-friendly. There is a
possibility that another trigger changes NEW values to "unmodified"
state after some conditional triggers are executed, but it could be
admissible. The approach 3. seems to be the most strict, but hard to
use because of the restriction.

----
Just for reference:

- Oracle Database:   They support multiple triggers and UPDATE OF and WHEN clause and   can modify NEW values in
triggerbodies. So they must have same   problems discussing here -- but I cannot find how they work around it...
http://download.oracle.com/docs/cd/B28359_01/appdev.111/b28370/create_trigger.htm#i2064026

- MySQL:   They can modify NEW values, but no problem because they don't support   UPDATE OF, WHEN clause, nor multiple
triggersfor each event.   http://dev.mysql.com/doc/refman/5.4/en/create-trigger.html
 


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

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

> Itagaki-san, isn't it more suitable to check rte->modifiedCols
> than heap_tuple_attr_equals()? Although, this information is
> not delivered to executor...

I'd like to check conditions by comparing actual values but not
a target of UPDATE statement because I think almost user expects
the former behavior. Unmodified UPDATE-targets are common case
if we use a framework that generates SQL statements internally.

Anyway, we need to compare the actual values if we want to treat
NEW value modifed by another trigger correctly.


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

On Thu, 2009-09-03 at 16:25 +0900, Itagaki Takahiro wrote:
> I'd like to check conditions by comparing actual values but not
> a target of UPDATE statement because I think almost user expects
> the former behavior. Unmodified UPDATE-targets are common case
> if we use a framework that generates SQL statements internally.

The SQL standard specifies that a trigger is fired if the column is
mentioned in the UPDATE statement, independent of whether the value is
actually changed through the update.

On Sep 3, 2009, at 7:44 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

> On Thu, 2009-09-03 at 16:25 +0900, Itagaki Takahiro wrote:
>> I'd like to check conditions by comparing actual values but not
>> a target of UPDATE statement because I think almost user expects
>> the former behavior. Unmodified UPDATE-targets are common case
>> if we use a framework that generates SQL statements internally.
>
> The SQL standard specifies that a trigger is fired if the column is
> mentioned in the UPDATE statement, independent of whether the value is
> actually changed through the update.

That is thorougly bizarre, IMO.

...Robert

Robert Haas wrote:
> On Wed, Sep 2, 2009 at 9:52 PM, Itagaki
> Takahiro<itagaki.takahiro@oss.ntt.co.jp> wrote:
>   
>> Here is a patch to implement "Support triggers on columns" in our ToDo list.
>>
>> The syntax is:
>>    CREATE TRIGGER name
>>        BEFORE UPDATE OF col1, col12, ...
>>        ON tbl FOR EACH ROW EXECUTE PROCEDURE func();
>>
>> I consulted the previous work following:
>>    Column-level triggers (From: Greg Sabino Mullane, Date: 2005-07-04)
>>    http://archives.postgresql.org/pgsql-patches/2005-07/msg00107.php
>> and completed some under-construction parts.
>>
>> It's still arguable that we should add dependencies from column
>> triggers to referenced columns. In the present patch, dropeed
>> columns are just ignored and always considered as not-modified.
>> Please grep with "TODO: (TRIGGER)" to check the issue.
>>
>> Comments welcome.
>>     
>
> Wow, so I wouldn't have to do this any more?
>
> IF (TG_OP = 'UPDATE') THEN
>     IF (OLD.foo IS NOT DISTINCT FROM NEW.foo AND OLD.bar IS NOT
> DISTINCT FROM NEW.bar
>          AND OLD.baz IS NOT DISTINCT FROM NEW.baz) THEN
>         RETURN NULL;
>     END IF;
> END IF;
>
> Apart from any possible gain in efficiency, the sheer savings in
> typing sound quite awesome.
>
>
>   

You could make it nicer with something like:
  row(new.foo,new.bar,new.baz) is distinct from 
row(old.foo,old.bar,old.baz)

couldn't you?

I'm actually having trouble thinking of a case where I'd find this 
feature very useful.

cheers

andrew

On Thu, 2009-09-03 at 07:57 -0400, Robert Haas wrote:
> On Sep 3, 2009, at 7:44 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> > The SQL standard specifies that a trigger is fired if the column is
> > mentioned in the UPDATE statement, independent of whether the value is
> > actually changed through the update.
> 
> That is thorougly bizarre, IMO.

Well, if you find that bizarre, consider the existing behavior: Why
should an ON UPDATE row trigger fire when none of the values of the
row's columns actually change?  I think if you read

TRIGGER ON UPDATE

as

TRIGER ON UPDATE OF <all columns>

then it makes some sense.

On Thu, Sep 3, 2009 at 9:51 AM, Peter Eisentraut<peter_e@gmx.net> wrote:
> On Thu, 2009-09-03 at 07:57 -0400, Robert Haas wrote:
>> On Sep 3, 2009, at 7:44 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> > The SQL standard specifies that a trigger is fired if the column is
>> > mentioned in the UPDATE statement, independent of whether the value is
>> > actually changed through the update.
>>
>> That is thorougly bizarre, IMO.
>
> Well, if you find that bizarre, consider the existing behavior: Why
> should an ON UPDATE row trigger fire when none of the values of the
> row's columns actually change?  I think if you read
>
> TRIGGER ON UPDATE
>
> as
>
> TRIGER ON UPDATE OF <all columns>
>
> then it makes some sense.

Not to me.  I use triggers to maintain database invariants, such as:

CREATE TABLE foo (id serial, name varchar, number_of_bars integer not
null default 0, primary key (id));
CREATE TABLE bar (id serial, foo_id integer not null references foo (id));

By setting up INSERT, UPDATE, and DELETE triggers on bar, I can
maintain the invariant that number_of_bars for each foo is in fact the
number of bars where foo_id is the id of that foo.  However, in order
to suppress unnecessary updates to the foo table, I have to have the
update trigger check whether OLD.foo_id = NEW.foo_id before it does
anything.

If TRIGGER ON UPDATE OF foo_id means whether the value actually
changed, then I can skip the check.  If TRIGGER ON UPDATE OF foo_id
means whether the column was present in the update list, then it
doesn't.  Perhaps there are some use cases where we can be certain
that we only care about whether the value was in the update list, and
not whether it was changed, but off the top of my head it seems like
0% of mine would fall into that category.

It also seems to me logically inconsistent that we would expose this
information via the CREATE TRIGGER interface but not to the trigger
function itself.  From within the function, you can compare NEW and
OLD, but you get no visibility into which columns were actually
updated.  And apparently now from within CREATE TRIGGER we'll have
just the opposite.  Blech...

By the way, I completely agree that it would be useful to have a way
to suppress triggers from firing when no columns were actually
modified.  But I also wouldn't argue that should be the only available
behavior.  Sometimes it's useful to schedule a no-op update explicitly
for the purpose of firing triggers.

...Robert

On Thu, 2009-09-03 at 10:24 -0400, Robert Haas wrote:
> If TRIGGER ON UPDATE OF foo_id means whether the value actually
> changed, then I can skip the check.  If TRIGGER ON UPDATE OF foo_id
> means whether the column was present in the update list, then it
> doesn't.  Perhaps there are some use cases where we can be certain
> that we only care about whether the value was in the update list, and
> not whether it was changed, but off the top of my head it seems like
> 0% of mine would fall into that category.

Yeah, probably.  I didn't make this up; I'm just reading the
standard. ;-)

But of course you can already do what you do, so you don't lose anything
if it turns out that this proposed feature ends up working the other
way.

> It also seems to me logically inconsistent that we would expose this
> information via the CREATE TRIGGER interface but not to the trigger
> function itself.  From within the function, you can compare NEW and
> OLD, but you get no visibility into which columns were actually
> updated.  And apparently now from within CREATE TRIGGER we'll have
> just the opposite.  Blech...

Well, it might make sense to make this information available within the
trigger function through new variables.

Robert Haas <robertmhaas@gmail.com> wrote: 
> It also seems to me logically inconsistent that we would expose this
> information via the CREATE TRIGGER interface but not to the trigger
> function itself.  From within the function, you can compare NEW and
> OLD, but you get no visibility into which columns were actually
> updated.  And apparently now from within CREATE TRIGGER we'll have
> just the opposite.  Blech...
Sybase provides an "if update(columnname)" syntax to allow such tests.
Perhaps PostgreSQL could do something similar?
> Sometimes it's useful to schedule a no-op update explicitly for the
> purpose of firing triggers.
Yes.  It's a less frequent need, but it does exist.  The thing is, if
you only fire triggers if something was actually changed to a new
value, you can't get to that.  If you fire on all updates you can test
whether there were actual changes.  Of course, ideally, both would be
convenient.
-Kevin

Hi,

Robert Haas <robertmhaas@gmail.com> writes:
> By the way, I completely agree that it would be useful to have a way
> to suppress triggers from firing when no columns were actually
> modified.  
 http://www.postgresql.org/docs/8.4/static/functions-trigger.html
 Currently PostgreSQL provides one built in trigger function, suppress_redundant_updates_trigger, which will prevent
anyupdate that does not actually change the data in the row from taking place, in contrast to the normal behaviour
whichalways performs the update regardless of whether or not the data has changed. (This normal behaviour makes updates
runfaster, since no checking is required, and is also useful in certain cases.)
 
 ...
 The suppress_redundant_updates_trigger function can be added to a table like this:
 CREATE TRIGGER z_min_update  BEFORE UPDATE ON tablename FOR EACH ROW EXECUTE PROCEDURE
suppress_redundant_updates_trigger();

Regards,
-- 
dim

On Thu, Sep 3, 2009 at 10:37 AM, Peter Eisentraut<peter_e@gmx.net> wrote:
> On Thu, 2009-09-03 at 10:24 -0400, Robert Haas wrote:
>> If TRIGGER ON UPDATE OF foo_id means whether the value actually
>> changed, then I can skip the check.  If TRIGGER ON UPDATE OF foo_id
>> means whether the column was present in the update list, then it
>> doesn't.  Perhaps there are some use cases where we can be certain
>> that we only care about whether the value was in the update list, and
>> not whether it was changed, but off the top of my head it seems like
>> 0% of mine would fall into that category.
>
> Yeah, probably.  I didn't make this up; I'm just reading the
> standard. ;-)
>
> But of course you can already do what you do, so you don't lose anything
> if it turns out that this proposed feature ends up working the other
> way.

Sure, but I don't think it makes a lot of sense to spend a lot of time
implementing the standard behavior unless someone can provide a
plausible use case.  If that means we have to give our non-standard
feature an incompatible syntax or whatever so as not to create
confusion with the "standard" behavior, then let's do that, because it
sounds WAY more useful.

...Robert

On Thu, 3 Sep 2009, Robert Haas wrote:

> On Thu, Sep 3, 2009 at 9:51 AM, Peter Eisentraut<peter_e@gmx.net> wrote:
>> On Thu, 2009-09-03 at 07:57 -0400, Robert Haas wrote:
>>> On Sep 3, 2009, at 7:44 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
>>>> The SQL standard specifies that a trigger is fired if the column is
>>>> mentioned in the UPDATE statement, independent of whether the value is
>>>> actually changed through the update.
>>>
>>> That is thorougly bizarre, IMO.
>>
>> Well, if you find that bizarre, consider the existing behavior: Why
>> should an ON UPDATE row trigger fire when none of the values of the
>> row's columns actually change?  I think if you read
>>
>> TRIGGER ON UPDATE
>>
>> as
>>
>> TRIGER ON UPDATE OF <all columns>
>>
>> then it makes some sense.
>
> Not to me.  I use triggers to maintain database invariants, such as:
>
> CREATE TABLE foo (id serial, name varchar, number_of_bars integer not
> null default 0, primary key (id));
> CREATE TABLE bar (id serial, foo_id integer not null references foo (id));
>
> By setting up INSERT, UPDATE, and DELETE triggers on bar, I can
> maintain the invariant that number_of_bars for each foo is in fact the
> number of bars where foo_id is the id of that foo.  However, in order
> to suppress unnecessary updates to the foo table, I have to have the
> update trigger check whether OLD.foo_id = NEW.foo_id before it does
> anything.
>
> If TRIGGER ON UPDATE OF foo_id means whether the value actually
> changed, then I can skip the check.  If TRIGGER ON UPDATE OF foo_id
> means whether the column was present in the update list, then it
> doesn't.  Perhaps there are some use cases where we can be certain
> that we only care about whether the value was in the update list, and
> not whether it was changed, but off the top of my head it seems like
> 0% of mine would fall into that category.
>
> It also seems to me logically inconsistent that we would expose this
> information via the CREATE TRIGGER interface but not to the trigger
> function itself.  From within the function, you can compare NEW and
> OLD, but you get no visibility into which columns were actually
> updated.  And apparently now from within CREATE TRIGGER we'll have
> just the opposite.  Blech...
>
> By the way, I completely agree that it would be useful to have a way
> to suppress triggers from firing when no columns were actually
> modified.  But I also wouldn't argue that should be the only available
> behavior.  Sometimes it's useful to schedule a no-op update explicitly
> for the purpose of firing triggers.

A simple use case would be to update a timestamp column with CURRENT_TIMESTAMP 
as instance.

>
> ...Robert

-- 
Guillaume (ioguix) de Rorthais

ioguix@free.fr escribió:

> A simple use case would be to update a timestamp column with
> CURRENT_TIMESTAMP as instance.

No, because you want to update the timestamp in all cases, whatever
columns the update actually updates.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On tor, 2009-09-03 at 11:19 -0400, Robert Haas wrote:
> Sure, but I don't think it makes a lot of sense to spend a lot of time
> implementing the standard behavior unless someone can provide a
> plausible use case.

One use case is porting Oracle applications.  I see a lot of that used
there.  The original proposer might had have other ideas.

On Thu, Sep 3, 2009 at 2:16 PM, Peter Eisentraut<peter_e@gmx.net> wrote:
> On tor, 2009-09-03 at 11:19 -0400, Robert Haas wrote:
>> Sure, but I don't think it makes a lot of sense to spend a lot of time
>> implementing the standard behavior unless someone can provide a
>> plausible use case.
>
> One use case is porting Oracle applications.  I see a lot of that used
> there.  The original proposer might had have other ideas.

Perhaps so, but his second post to the thread suggests that he didn't
have the interpretation you're proposing in mind.

...Robert

Peter Eisentraut <peter_e@gmx.net> writes:
> On tor, 2009-09-03 at 11:19 -0400, Robert Haas wrote:
>> Sure, but I don't think it makes a lot of sense to spend a lot of time
>> implementing the standard behavior unless someone can provide a
>> plausible use case.

> One use case is porting Oracle applications.  I see a lot of that used
> there.  The original proposer might had have other ideas.

That's only a good argument if we are prepared to implement exactly
Oracle's semantics for the feature ... which, frankly, I have no reason
whatever to assume are exactly like the standard's :-(
        regards, tom lane

Peter Eisentraut <peter_e@gmx.net> wrote:

> The SQL standard specifies that a trigger is fired if the column is
> mentioned in the UPDATE statement, independent of whether the value is
> actually changed through the update.

Hmmm, what does the SQL standard say about modification of NEW values?
Should we fire column triggers when their columns are mentioned in the
UPDATE statement, but modified by another triggers?

I believe we should fire them, but it is inconsistent because we will
make different decisions whether NEW values are modified or not.

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

Here is a updated version of column-trigger patch.

Changes from the previous patch:
  * Add dependency of columns with recordDependencyOn().
    Regression tests are also adjusted.
  * Recheck columns if NEW values are modified, but each trigger
    will be firec only one per row.


Peter Eisentraut <peter_e@gmx.net> wrote:
> The SQL standard specifies that a trigger is fired if the column is
> mentioned in the UPDATE statement, independent of whether the value is
> actually changed through the update.

We are discussing how to determine modified columns
(UPDATE-target vs. changes of actual values), but in the patch
I used value-based checking. The reasons are:
    1. Other triggers could modify referred columns even if the columns
       are not specifed in UPDATE-target.
    2. IMHO, almost users don't expect their triggers are not called
       if the actual values are not modified.
    3. Restriction of implementation; We don't have RTE in trigger
       routine for now. The current patch doesn't modify codes a lot.

Comments welcome.

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

On Mon, 2009-09-07 at 11:20 +0900, Itagaki Takahiro wrote:
> We are discussing how to determine modified columns
> (UPDATE-target vs. changes of actual values), but in the patch
> I used value-based checking. The reasons are:

If you implement a new feature using syntax from the standard, you have
to implement the semantics of the standard.  If you don't like the
semantics of the standard, use a different syntax.

>     2. IMHO, almost users don't expect their triggers are not called
>        if the actual values are not modified.

Well, as we saw upthread, there can be different valid opinions on this.
But consider the following:

- Statement triggers are called even if the table was not actually
changed in a semantically significant way.

- Row triggers are called even if the row was not actually changed in a
semantically significant way.

Therefore, it cannot be completely unexpected if column triggers are
called even if the column was not actually changed in a semantically
significant way.

Peter Eisentraut <peter_e@gmx.net> wrote:

> Therefore, it cannot be completely unexpected if column triggers are
> called even if the column was not actually changed in a semantically
> significant way.

Ok, the attached patch implements standard-compliant version of
column trigger.

Retrieving modified columns is not so difficult as I expected.
It is in:
  rt_fetch(relinfo->ri_RangeTableIndex, estate->es_range_table)->modifiedCols
and the information are passed from caller to trigger routines.


However, to be honest, I think standard-compliant column trigger is
useless... I'm thinking additional extension for triggers -- if we
want to check modifications of actual values, it could be defined as:

    CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW
        WHEN (NEW.col <> OLD.col) EXECUTE PROCEDURE trigger_func();

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

On Mon, Sep 07, 2009 at 07:53:01PM +0900, Itagaki Takahiro wrote:
> However, to be honest, I think standard-compliant column trigger is
> useless... I'm thinking additional extension for triggers -- if we
> want to check modifications of actual values, it could be defined
> as:
> 
>     CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW
>         WHEN (NEW.col <> OLD.col) EXECUTE PROCEDURE trigger_func();

That should probably read:

CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW   WHEN (NEW.col IS DISTINCT FROM OLD.col) EXECUTE PROCEDURE
trigger_func();

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

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

David Fetter <david@fetter.org> wrote: 
> CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW
>     WHEN (NEW.col IS DISTINCT FROM OLD.col)
>         EXECUTE PROCEDURE trigger_func();
How much does that buy you versus including this at the start of
trigger_func:
IF (NEW.col IS NOT DISTINCT FROM OLD.col) THEN RETURN NEW;
END IF;
What about the desire (mentioned up-thread) to test whether a column
was the target of an update SET list within the trigger function?
-Kevin

On Sep 8, 2009, at 7:38 AM, Kevin Grittner wrote:

> David Fetter <david@fetter.org> wrote:
>
>> CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW
>>    WHEN (NEW.col IS DISTINCT FROM OLD.col)
>>        EXECUTE PROCEDURE trigger_func();
>
> How much does that buy you versus including this at the start of
> trigger_func:

On the face, it buys nothing, IMO. ISTM, looking at the examples, that  
the above syntax would lead to redundant logic if the particular  
trigger_func() were used by multiple TRIGGERs. That is, assuming the  
precondition is necessary for proper functionality, it would have to  
be repeated on all the TRIGGERs that trigger_func() would be executed  
by.

[..moving away from the isolated use-case of the example]

However, if trigger_func() were a generalized trigger function, which  
would likely be the case if it were used by multiple TRIGGERs[;)]. The  
necessary precondition would probably be inconsistent across the  
TRIGGERs, and thus the feature could be quite useful.
Currently, such a generalized trigger function *could* be crafted  
using trigger arguments, but I'd be inclined to think that that would  
require more exercise than it would be worth( that is, I'm imagining  
something that would be dirty, and much less convenient than WHEN =).


Personally, I think WHEN () would be pretty sweet. =)

On Mon, Sep 7, 2009 at 6:53 AM, Itagaki
Takahiro<itagaki.takahiro@oss.ntt.co.jp> wrote:
>
> Peter Eisentraut <peter_e@gmx.net> wrote:
>
>> Therefore, it cannot be completely unexpected if column triggers are
>> called even if the column was not actually changed in a semantically
>> significant way.
>
> Ok, the attached patch implements standard-compliant version of
> column trigger.
>
> Retrieving modified columns is not so difficult as I expected.
> It is in:
>  rt_fetch(relinfo->ri_RangeTableIndex, estate->es_range_table)->modifiedCols
> and the information are passed from caller to trigger routines.
>
>
> However, to be honest, I think standard-compliant column trigger is
> useless... I'm thinking additional extension for triggers -- if we
> want to check modifications of actual values, it could be defined as:
>
>    CREATE TRIGGER trig BEFORE UPDATE ON tbl FOR EACH ROW
>        WHEN (NEW.col <> OLD.col) EXECUTE PROCEDURE trigger_func();

It might be better to use WHERE than WHEN.  Unfortunately this still
has all the wordiness of doing it in the trigger function.  How about
some more trivial syntax modification, like:

CREATE TRIGGER name      BEFORE UPDATE MODIFYING col1, col12, ...      ON tbl FOR EACH ROW EXECUTE PROCEDURE func();

...Robert

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:

> How much does that buy you versus including this at the start of
> trigger_func:

One of the benefits is that it could handle tuple modifications
by another trigger, that is discussed here: http://archives.postgresql.org/pgsql-hackers/2009-09/msg00161.php | 2.
Recheckconditions if NEW values are modified, but triggers that |    have been fired already are not executed twice.
 

In addition, some database developers think procedures of trigger
and its condition separately. They might use other DBMSs that supports
WHEN clause in other times [1][2][3]. (All of them use "WHEN" for the
syntax; that's why I proposed "WHEN" but not "WHERE".)

Also, it would be useful if we reuse trigger bodies multiple times
with different conditions.

[1] Oracle   http://download.oracle.com/docs/cd/B28359_01/appdev.111/b28370/create_trigger.htm
[2] IBM DB2
http://publib.boulder.ibm.com/infocenter/db2luw/v9/index.jsp?topic=/com.ibm.db2.udb.admin.doc/doc/r0000931.htm
[3] SQLite3   http://www.sqlite.org/lang_createtrigger.html

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

On Sep 8, 2009, at 5:33 PM, Itagaki Takahiro wrote:
> WHEN clause in other times [1][2][3]. (All of them use "WHEN" for the
> syntax; that's why I proposed "WHEN" but not "WHERE".)

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)

On Tue, Sep 08, 2009 at 06:28:36PM -0700, James Pye wrote:
> On Sep 8, 2009, at 5:33 PM, Itagaki Takahiro wrote:
>> WHEN clause in other times [1][2][3]. (All of them use "WHEN" for
>> the syntax; that's why I proposed "WHEN" but not "WHERE".)
>
> 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)

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>

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

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

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> wrote:

> Ok, the attached patch implements standard-compliant version of
> column trigger.

Here is an updated version of column-level trigger patch.
I forgot to adjust pg_get_triggerdef() in the previous version.
pg_dump also uses pg_get_triggerdef() instead of building
CREATE TRIGGER statements to avoid duplicated codes if the
server version is 8.5 or later.

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

On Mon, 2009-09-14 at 18:58 +0900, Itagaki Takahiro wrote:
> Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> wrote:
> 
> > Ok, the attached patch implements standard-compliant version of
> > column trigger.
> 
> Here is an updated version of column-level trigger patch.
> I forgot to adjust pg_get_triggerdef() in the previous version.
> pg_dump also uses pg_get_triggerdef() instead of building
> CREATE TRIGGER statements to avoid duplicated codes if the
> server version is 8.5 or later.

What is the purpose of the new pg_get_triggerdef() variant?  OK, the
parameter name "pretty_bool" gives a hint, but what does this have to do
with column triggers?  Maybe you could try to explain this in more
detail.  Ideally split the patch into two: one that deals with
pg_get_triggerdef(), and one that deals with column triggers.

If you want a "pretty" option on pg_get_triggerdef(), you could nowadays
also implement that via a parameter default value instead of a second
function.

Peter Eisentraut <peter_e@gmx.net> wrote:

> What is the purpose of the new pg_get_triggerdef() variant?  OK, the
> parameter name "pretty_bool" gives a hint, but what does this have to do
> with column triggers?  Maybe you could try to explain this in more
> detail.  Ideally split the patch into two: one that deals with
> pg_get_triggerdef(), and one that deals with column triggers.

It's for pg_dump. We can avoid duplicated codes if we use
pg_get_triggerdef() in pg_dump. So, I think column trigger and
the dump function for column trigger should be applied at once.

> If you want a "pretty" option on pg_get_triggerdef(), you could nowadays
> also implement that via a parameter default value instead of a second
> function.

OK, I'll rewrite it to use default parameter.

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

> Peter Eisentraut <peter_e@gmx.net> wrote:
> > If you want a "pretty" option on pg_get_triggerdef(), you could nowadays
> > also implement that via a parameter default value instead of a second
> > function.
> 
> OK, I'll rewrite it to use default parameter.

I tried to remove pg_get_triggerdef_ext() and add a second argument
with default value to pg_get_triggerdef(), but there is a problem.

The definition of pg_get_triggerdef will be the following:   DATA(insert OID = 1662 (       pg_get_triggerdef    PGNSP
PGUID12 1 0 0 f f f t f s 2 1 25 "26 16" _null_ _null_ _null_       "({CONST :consttype 16 :consttypmod -1 :constlen 1
:constbyvaltrue :constisnull false :location 41 :constvalue 1 [ 0 0 0 0 0 0 0 0 ]})"       pg_get_triggerdef _null_
_null__null_ ));
 

The problem is in :constvalue part; It will be
":constvalue 1 [ 0 0 0 0 0 0 0 0 ]" on 64bit platform, but
":constvalue 1 [ 0 0 0 0 ]" on 32bit platform.
I think we should not use default values in functions listed on pg_proc.h,
so the previous patch is better than default value version.

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

On Thu, 2009-10-01 at 10:40 +0900, Itagaki Takahiro wrote:
> > Peter Eisentraut <peter_e@gmx.net> wrote:
> > > If you want a "pretty" option on pg_get_triggerdef(), you could nowadays
> > > also implement that via a parameter default value instead of a second
> > > function.
> > 
> > OK, I'll rewrite it to use default parameter.
> 
> I tried to remove pg_get_triggerdef_ext() and add a second argument
> with default value to pg_get_triggerdef(), but there is a problem.
> 
> The definition of pg_get_triggerdef will be the following:
>     DATA(insert OID = 1662 (
>         pg_get_triggerdef    PGNSP PGUID 12 1 0 0 f f f t f s 2 1 25 "26 16" _null_ _null_ _null_
>         "({CONST :consttype 16 :consttypmod -1 :constlen 1 :constbyval true :constisnull false :location 41
:constvalue1 [ 0 0 0 0 0 0 0 0 ]})"
 
>         pg_get_triggerdef _null_ _null_ _null_ ));
> 
> The problem is in :constvalue part; It will be
> ":constvalue 1 [ 0 0 0 0 0 0 0 0 ]" on 64bit platform, but
> ":constvalue 1 [ 0 0 0 0 ]" on 32bit platform.
> I think we should not use default values in functions listed on pg_proc.h,
> so the previous patch is better than default value version.

OK, but what you can do is point both variants to the same C function
and check with PG_NARGS() with how many arguments you were called.  That
would save some of the indirections.

Peter Eisentraut <peter_e@gmx.net> wrote:

> OK, but what you can do is point both variants to the same C function
> and check with PG_NARGS() with how many arguments you were called.  That
> would save some of the indirections.

The regressiontest 'opr_sanity' failed if do so. Should we remove this
check only for pg_get_triggerdef? If we cannot do that, the first version
of patch is still the best solution.

-- Considering only built-in procs (prolang = 12), look for multiple uses
-- of the same internal function (ie, matching prosrc fields).  It's OK to
-- have several entries with different pronames for the same internal function,
-- but conflicts in the number of arguments and other critical items should
-- be complained of.  (We don't check data types here; see next query.)
-- Note: ignore aggregate functions here, since they all point to the same
-- dummy built-in function.
  oid  |      proname      | oid  |      proname      
! ------+-------------------+------+-------------------
!  1662 | pg_get_triggerdef | 2730 | pg_get_triggerdef
! (1 row)

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

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Peter Eisentraut <peter_e@gmx.net> wrote:
>> OK, but what you can do is point both variants to the same C function
>> and check with PG_NARGS() with how many arguments you were called.  That
>> would save some of the indirections.

> The regressiontest 'opr_sanity' failed if do so. Should we remove this
> check only for pg_get_triggerdef? If we cannot do that, the first version
> of patch is still the best solution.

I have always been of the opinion that V1 functions should be written
in the style

foo(PG_FUNCTION_ARGS)
{type1    arg1 = PG_GETARG_whatever(0);type2    arg2 = PG_GETARG_whatever(1);type3    arg3 = PG_GETARG_whatever(2);

as much as possible.  The V1 protocol is already a big hit to
readability compared to plain-vanilla C functions, and one of the main
reasons is that you can't instantly see what arguments a function is
expecting.  Sticking to the above style ameliorates that.  Cute tricks
like conditionally grabbing arguments depending on PG_NARGS do far more
damage to readability than they can ever repay in any other metric.

In short: while I haven't looked at the patch, I think Peter may be
steering you in the wrong direction.

In cases where you do have related functions, I suggest having
SQL-callable V1 functions that absorb their arguments in this
style, and then have them call a common subroutine that's a plain
C function.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> In cases where you do have related functions, I suggest having
> SQL-callable V1 functions that absorb their arguments in this
> style, and then have them call a common subroutine that's a plain
> C function.

Unless you have high performance requirements, IME. Avoiding the SQL
function call is indeed measurable, even if very low in the radar.

Regards,
-- 
dim

You have a nice quote about the sins we'd accept/follow in the name of
performance, but google will about only find sin/cos etc material...

On Sun, 2009-10-04 at 22:07 -0400, Tom Lane wrote:
> In short: while I haven't looked at the patch, I think Peter may be
> steering you in the wrong direction.
> 
> In cases where you do have related functions, I suggest having
> SQL-callable V1 functions that absorb their arguments in this
> style, and then have them call a common subroutine that's a plain
> C function.

Yeah, that's what he did.  So forget what I said. :-)

2009/10/7 Peter Eisentraut <peter_e@gmx.net>:
> On Sun, 2009-10-04 at 22:07 -0400, Tom Lane wrote:
>> In short: while I haven't looked at the patch, I think Peter may be
>> steering you in the wrong direction.
>>
>> In cases where you do have related functions, I suggest having
>> SQL-callable V1 functions that absorb their arguments in this
>> style, and then have them call a common subroutine that's a plain
>> C function.
>
> Yeah, that's what he did.  So forget what I said. :-)
>

What's the current status of this patch in the commitfest process?  I
see the last version posted by the author was on 14 Sep and there's
been some discussion since then, but no new patch version has
surfaced.  Are we waiting on a new version of the patch, or is there
more review of the existing version yet to be done?  The status of the
patch on the commitfest app is still "Needs Review", but I'm not sure
whether that is accurate.

Cheers,
BJ

On Thu, 2009-10-08 at 08:57 +1100, Brendan Jurd wrote:
> What's the current status of this patch in the commitfest process?

I'm working on it.

On Mon, 2009-09-14 at 18:58 +0900, Itagaki Takahiro wrote:
> Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> wrote:
> 
> > Ok, the attached patch implements standard-compliant version of
> > column trigger.
> 
> Here is an updated version of column-level trigger patch.
> I forgot to adjust pg_get_triggerdef() in the previous version.
> pg_dump also uses pg_get_triggerdef() instead of building
> CREATE TRIGGER statements to avoid duplicated codes if the
> server version is 8.5 or later.

I have committed the parts involving pg_get_triggerdef and pg_dump.  I
will get to the actual column trigger functionality next.

On Sat, 2009-10-10 at 00:04 +0300, Peter Eisentraut wrote:
> On Mon, 2009-09-14 at 18:58 +0900, Itagaki Takahiro wrote:
> > Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> wrote:
> >
> > > Ok, the attached patch implements standard-compliant version of
> > > column trigger.
> >
> > Here is an updated version of column-level trigger patch.
> > I forgot to adjust pg_get_triggerdef() in the previous version.
> > pg_dump also uses pg_get_triggerdef() instead of building
> > CREATE TRIGGER statements to avoid duplicated codes if the
> > server version is 8.5 or later.
>
> I have committed the parts involving pg_get_triggerdef and pg_dump.  I
> will get to the actual column trigger functionality next.

Attached is a merged up patch with some slightly improved documentation.

I think the patch is almost ready now.  One remaining issue is, in
TriggerEnabled() you apparently check the column list only if it is a
row trigger.  But columns are supported for statement triggers as well
per SQL standard.  Check please.

Btw., I might not get a chance to commit this within the next 48 hours.
If someone else wants to, go ahead.

Peter Eisentraut <peter_e@gmx.net> wrote:

> I think the patch is almost ready now.  One remaining issue is, in
> TriggerEnabled() you apparently check the column list only if it is a
> row trigger.  But columns are supported for statement triggers as well
> per SQL standard.  Check please.

I added it. I've missed it because I tried to implement column
trigger based on value-comparison, but now we check conditions
based on a target-list. Statement column triggers are reasonable.

diff-of-column-trigger.patch is a diff from the previous column-trigger.patch.

column-trigger_20091014.patch is a full patch to the HEAD.

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

Peter Eisentraut <peter_e@gmx.net> writes:
> Btw., I might not get a chance to commit this within the next 48 hours.
> If someone else wants to, go ahead.

I will take it.
        regards, tom lane

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> column-trigger_20091014.patch is a full patch to the HEAD.

Applied with assorted corrections, mostly cosmetic but not entirely.
One thing you really should have caught was updating
copyfuncs/equalfuncs for the parsenode field addition.
Next time, try grepping for every reference to a struct type you
change ...
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Applied with assorted corrections, mostly cosmetic but not entirely.
> One thing you really should have caught was updating
> copyfuncs/equalfuncs for the parsenode field addition.
> Next time, try grepping for every reference to a struct type you
> change ...

Thanks for the fix. I'll be careful about it.

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