Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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.
Attachment
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
Attachment
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