The same trigger is being used for multiple operations. Bug?

[cowwoc]

Hi,

There seems to be some confusion in the specification of
org.h2.api.Trigger. If you invoke:

CREATE TRIGGER foo_history_trigger AFTER INSERT, UPDATE, DELETE ON foo
FOR EACH ROW CALL
"MyTrigger";

H2 invokes init() with type = 7. If you look at the Javadoc of the
"type" parameter it says: "the operation type: INSERT, UPDATE, or
DELETE".

I am expecting a Trigger to belong to *one* of those types. In fact,
if you look at the original RFE that led to this feature getting
added, they had the same thing in mind:
http://groups.google.com/group/h2-database/browse_frm/thread/c510e6ce182826a2/72963a2273c49c97#72963a2273c49c97

Proposal:

1. Either instantiate one Trigger per type (better from a backwards-
compatibility point of view), or
2. Add a new "type" parameter to the fire() method so we can act on
the actual type that gets fired (better from a performance point of
view). I would keep the "type" parameter in init() in case someone
needs to initialize differently depending on the range of operations a
trigger may handle.

Either way, the Javadoc should match the actual behavior.

Thanks,
Gili

[Rami Ojares]

+1 to this.
Knowing at initialization time that the trigger has been defined for DELETE, UPDATE and INSERT
has no use at least I have not found any.

If you use the same class for different types of events then chances are that there is only some
slight variations in the behaviour based on the event type and these will be handled in the fire method.

But currently the fire method does not know what event triggered it.

So instead of telling the init method all possible events we should tell the fire method
the actual event that triggered it.

Workaround:

Instead of

CREATE TRIGGER foo_history_trigger AFTER INSERT, UPDATE, DELETE ON foo FOR EACH ROW CALL "MyTrigger";

Use
CREATE TRIGGER foo_history_trigger_i AFTER INSERT ON foo FOR EACH ROW CALL "MyTrigger";
CREATE TRIGGER foo_history_trigger_u AFTER UPDATE ON foo FOR EACH ROW CALL "MyTrigger";
CREATE TRIGGER foo_history_trigger_d AFTER DELETE ON foo FOR EACH ROW CALL "MyTrigger";

Fix:
fire(Connection conn, Object[] oldRow, Object[] newRow, int type) throws SQLException

And here the type would be only one of INSERT, UPDATE and DELETE.

- rami

[Thomas Mueller]

Hi,

> I am expecting a Trigger to belong to *one* of those types.

I don't agree. It doesn't make sense to create one trigger object for
DELETE, one trigger object for INSERT, and one trigger object for
UPDATE.

> In fact,
> if you look at the original RFE that led to this feature getting
> added, they had the same thing in mind:

How did you come to this conclusion?

> 2. Add a new "type" parameter to the fire() method so we can act on
> the actual type that gets fired (better from a performance point of
> view).

That's possible, also for compatibility with HSQLDB. I guess it
doesn't make sense to change the existing interface (backwards
compatibility); instead, an Adapter style extension could be added.

Currently the following logic can be used:

if (oldRow != null) {
if (newRow != null) {
// update
if (hasChanged(oldRow, newRow, indexColumns)) {
delete(oldRow);
insert(newRow);
}
} else {
// delete
delete(oldRow);
}
} else if (newRow != null) {
// insert
insert(newRow);
}

> Either way, the Javadoc should match the actual behavior.

I will document in Trigger.init()

The type of operation is a bit field with the
appropriate flags set. As an example, if the trigger is of type INSERT
and UPDATE, then the parameter type is set to (INSERT | UPDATE).

@param type the operation type: INSERT, UPDATE, DELETE, SELECT, or a
combination (this parameter is a bit field)

Regards,
Thomas

[cowwoc]

Thomas,

Replies below...

On 06/07/2011 1:18 AM, Thomas Mueller wrote:
> Hi,
>
>> I am expecting a Trigger to belong to *one* of those types.
> I don't agree. It doesn't make sense to create one trigger object for
> DELETE, one trigger object for INSERT, and one trigger object for
> UPDATE.

Why? What are you worried about?

>> In fact,
>> if you look at the original RFE that led to this feature getting
>> added, they had the same thing in mind:
> How did you come to this conclusion?

MattShaw writes "We have logic in our triggers that uses this
information to determine what to do". My interpretation is that he'd
like to know the operation type in fire() in order to determine what to
do. After all, the bit-field passed into init() doesn't tell you what
specific operation that will trigger fire() so he couldn't have been
talking about that.

>> 2. Add a new "type" parameter to the fire() method so we can act on
>> the actual type that gets fired (better from a performance point of
>> view).
> That's possible, also for compatibility with HSQLDB. I guess it
> doesn't make sense to change the existing interface (backwards
> compatibility); instead, an Adapter style extension could be added.

Can you give an example of what you had in mind?

>
> Currently the following logic can be used:
>
> if (oldRow != null) {
> if (newRow != null) {
> // update
> if (hasChanged(oldRow, newRow, indexColumns)) {
> delete(oldRow);
> insert(newRow);
> }
> } else {
> // delete
> delete(oldRow);
> }
> } else if (newRow != null) {
> // insert
> insert(newRow);
> }

You've outlined a way for detecting whether fire() is handling
UPDATE, DELETE or INSERT. What about SELECT or ROLLBACK?

>
>> Either way, the Javadoc should match the actual behavior.
> I will document in Trigger.init()
>
> The type of operation is a bit field with the
> appropriate flags set. As an example, if the trigger is of type INSERT
> and UPDATE, then the parameter type is set to (INSERT | UPDATE).
>
> @param type the operation type: INSERT, UPDATE, DELETE, SELECT, or a
> combination (this parameter is a bit field)
>
> Regards,
> Thomas
>

Please also document how to detect the operation type in fire(),
not only in init().

Thanks,
Gili

[Thomas Mueller]

Hi,

>> I don't agree. It doesn't make sense to create one trigger object for
>> DELETE, one trigger object for INSERT, and one trigger object for
>> UPDATE.
>    Why? What are you worried about?

I'm not worried. It's just that it doesn't make sense to create
multiple objects. I expect one trigger object, not multiple. This is
how the fulltext index works for example.

>    MattShaw writes "We have logic in our triggers that uses this information
> to determine what to do". My interpretation is that he'd like to know the
> operation type in fire() in order to determine what to do.

This doesn't require one trigger object per action.

>> That's possible, also for compatibility with HSQLDB. I guess it
>> doesn't make sense to change the existing interface (backwards
>> compatibility); instead, an Adapter style extension could be added.
>
>    Can you give an example of what you had in mind?

See the HSQLDB documentation.

>    You've outlined a way for detecting whether fire() is handling UPDATE,
> DELETE or INSERT. What about SELECT or ROLLBACK?

Currently, there is no way to find out if a trigger was called because
a transaction was rolled back, that's true.

Regards,
Thomas

[cowwoc]

On 12/07/2011 5:32 PM, Thomas Mueller wrote:
>>> That's possible, also for compatibility with HSQLDB. I guess it
>>> doesn't make sense to change the existing interface (backwards
>>> compatibility); instead, an Adapter style extension could be added.
>> Can you give an example of what you had in mind?
> See the HSQLDB documentation.
>
>> You've outlined a way for detecting whether fire() is handling UPDATE,
>> DELETE or INSERT. What about SELECT or ROLLBACK?
> Currently, there is no way to find out if a trigger was called because
> a transaction was rolled back, that's true.

What about SELECT?

Anyway, I'm fine with whatever solution you come up with so long as
we can reliably detect the different trigger types are fire-time. Do you
plan on adding this functionality in the near future?

Thanks,
Gili