Proposal: Trigger2 Interface

[Gili]

Hi,

I'd like to propose a new Trigger interface (attached) with the following benefits:

• Simplified init() method: Users can pull as little or as much data as they'd like from class Metadata.
• Performance benefit for triggers that process multiple rows: PreparedStatements sharing using initTransaction(), closeTransaction().
• Ability to detect when triggers are fired by SELECT or ROLLBACK.
• Ability to clean up resources when a trigger is dropped (not just when the database is closed).

I initially shared this proposal with Noel a few months back and had hoped to implement it myself. Unfortunately, my work schedule has gotten completely out of hand and I no longer believe that I will be able to work on it after all. I am sharing it with the community in the hopes that you will pick up where I left off.

Kind regards,

Gili

[Thomas Mueller]

Hi,

Hm, it's interesting, but I'm not quite sure if the change is worth it. What are the pain points?

> ability to clean up resources when a trigger is dropped (not just when the database is closed)

There are already method Trigger.remove() and Trigger.close(). Your interface only has close(). So it's actually one step back from what we have now?

> Simplified init() method

Well, in a way you added complexity by adding a class. But I see your point, having 6 parameters isn't all that great. But this change alone isn't worth the trouble I think. Changing the API needs to have a real, big benefit.

> Performance benefit for triggers that process multiple rows

Sorry what is the performance benefit? Also, do you have a benchmark?

> Ability to detect when triggers are fired by SELECT or ROLLBACK

Well, what would you pass when rolling back an update in the fire method: UPDATE or ROLLBACK? In fact both should be passed...

Regards,

Thomas

--
You received this message because you are subscribed to the Google Groups "H2 Database" group.
To unsubscribe from this group and stop receiving emails from it, send an email to h2-database...@googlegroups.com.
To post to this group, send email to h2-da...@googlegroups.com.
Visit this group at http://groups.google.com/group/h2-database.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[cowwoc]

Hi Thomas,

On 17/07/2013 12:35 PM, Thomas Mueller wrote:

Hi,

Hm, it's interesting, but I'm not quite sure if the change is worth it. What are the pain points?

> ability to clean up resources when a trigger is dropped (not just when the database is closed)

There are already method Trigger.remove() and Trigger.close(). Your interface only has close(). So it's actually one step back from what we have now?

Hi Thomas,

    I am under the impression that it doesn't matter whether a Trigger's resources get cleaned up due to the Trigger being dropped or the database being closed. Do you have a use-case that counters that?

> Simplified init() method

Well, in a way you added complexity by adding a class. But I see your point, having 6 parameters isn't all that great. But this change alone isn't worth the trouble I think. Changing the API needs to have a real, big benefit.

    Today you have 6 parameters. Tomorrow you will have 12. This approach makes it easier to evolve the API in the future. This isn't an argument in favor of the new interface, it's just icing on the cake.

> Performance benefit for triggers that process multiple rows

Sorry what is the performance benefit? Also, do you have a benchmark?

    I am referring to the fact that currently if I want to process N rows, you need to create N * PreparedStatements (one per row inside fire()) as opposed to once at the beginning of the transaction. You are right that this needs to be benchmarked.

> Ability to detect when triggers are fired by SELECT or ROLLBACK

Well, what would you pass when rolling back an update in the fire method: UPDATE or ROLLBACK? In fact both should be passed...

    In the case you described (update followed by a rollback) the trigger would fire() once for UPDATE and once for ROLLBACK. There is also the conceptual benefit of the API explicitly stating the StatementType as opposed to having users reverse-engineer it from other parameters.

Gili

[Noel Grandin]

On 2013-07-17 19:51, cowwoc wrote:

    I am referring to the fact that currently if I want to process N rows, you need to create N * PreparedStatements (one per row inside fire()) as opposed to once at the beginning of the transaction. You are right that this needs to be benchmarked.

One change we could make right now without even having a new interface, would be to instantiate the Trigger objects on a per-connection basis.
That would allow the Trigger instances to create and cache PreparedStatements in their init() method.

[cowwoc]

    While technically speaking you could do so, practically speaking it isn't clear what impact this would have on implementations in the wild. The current interface states that init() provides a system connection. By changing the implementation you would be violating the contract of the original interface, though again it's not clear whether existing code would break.

Gili

[Thomas Mueller]

Hi,

> That would allow the Trigger instances to create and cache PreparedStatements in their init() method.

Yes, if you look at the FullTextTrigger class (package org.h2.fulltext), you can already cache the prepared statements, using this system session.

Regards,

Thomas

[Noel Grandin]

On Sat, Jul 20, 2013 at 3:11 PM, Thomas Mueller
<thomas.to...@gmail.com> wrote:
>
>> That would allow the Trigger instances to create and cache
>> PreparedStatements in their init() method.
>
> Yes, if you look at the FullTextTrigger class (package org.h2.fulltext), you
> can already cache the prepared statements, using this system session.
>

Yeah, but that can lead to deadlocks, because the PreparedStatements
execute on a different session from the current session.
e.g. see this previous discussion:
https://groups.google.com/d/topic/h2-database/B3xG488RBhI/discussion

[Thomas Mueller]

Hi,

> but that can lead to deadlocks ...

> e.g. see this previous discussion:

As part of that discussion, I wrote: "I suggest to use PreparedStatement, and always create a new PreparedStatement (for each invokation of the trigger). Internally, the database caches a low-level part of a PreparedStatement,..."

So, I wonder if caching prepared statements is really a problem?

> I am under the impression that it doesn't matter whether a Trigger's resources get cleaned up due to the Trigger being dropped or the database being closed. Do you have a use-case that counters that?

The use case is: you might want to drop a table when the trigger is removed, but do nothing if the database is closed.

Regards,

Thomas

[cowwoc]

Hi Thomas,

On 20/07/2013 1:10 PM, Thomas Mueller wrote:

Hi,

> but that can lead to deadlocks ...

> e.g. see this previous discussion:

As part of that discussion, I wrote: "I suggest to use PreparedStatement, and always create a new PreparedStatement (for each invokation of the trigger). Internally, the database caches a low-level part of a PreparedStatement,..."

So, I wonder if caching prepared statements is really a problem?

    I wrote a quick benchmark against an in-memory database that inserts a million rows, then drops them. I ran this against a trigger that creates a new PreparedStatement in fire() and with a trigger that creates a new PreparedStatement in init().

    Multiple PreparedStatement: 1.746 seconds
    One PreparedStatement        : 1.427 seconds

    So we're talking about an overhead of 0.319 ms per invocation. Please double check the attached benchmark to make sure I'm not doing anything wrong.

> I am under the impression that it doesn't matter whether a Trigger's resources get cleaned up due to the Trigger being dropped or the database being closed. Do you have a use-case that counters that?

The use case is: you might want to drop a table when the trigger is removed, but do nothing if the database is closed.

    Fair enough, so let's keep them separate.

Thanks,
Gili

[Thomas Mueller]

Hi,

In your test case, you didn't actually *execute* the statement. So it was "prepare a statement" versus "do nothing". Well, if the difference in time is so small, then I guess it doesn't make much sense to support this feature.

So, instead of continuing to discuss this back and forth, let's just keep the current trigger interface as it is, and whenever we do *have* to change it, then let's keep this discussion in mind. Specially, instead of passing 6 parameters, pass an object that contains that data (Metadata in your case). This was done in other places already: CreateTableData.

Regards,

Thomas

--

[cowwoc]

On 29/07/2013 4:53 PM, Thomas Mueller wrote:

Hi,

In your test case, you didn't actually *execute* the statement. So it was "prepare a statement" versus "do nothing". Well, if the difference in time is so small, then I guess it doesn't make much sense to support this feature.

    That was by design. I thought we were trying to measure the difference between preparing a statement once per transaction (as I was proposing) versus preparing it once per trigger fire(). Was that not the case?

So, instead of continuing to discuss this back and forth, let's just keep the current trigger interface as it is, and whenever we do *have* to change it, then let's keep this discussion in mind. Specially, instead of passing 6 parameters, pass an object that contains that data (Metadata in your case). This was done in other places already: CreateTableData.

Okay.

Gili

You received this message because you are subscribed to a topic in the Google Groups "H2 Database" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/h2-database/Sb3T1aVwoCE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to h2-database...@googlegroups.com.

[Thomas Mueller]

Hi,

> In your test case, you didn't actually *execute* the statement. So it was "prepare a statement" versus "do nothing"

I think a more realistic use case is: (a) prepare, bind the values, and execute a simple statement, versus (b) just bind the values and execute it.

Regards,

Thomas

[cowwoc]

Hi Thomas,

    Good thing you asked for the updated test, because I now see a huge performance difference:

Multiple PreparedStatement: 4.623 seconds
One PreparedStatement        : 1.944 seconds

    I've attached the updated testcase for your review.

    On a side-note, it took me a while to track down a deadlock for the second case. H2 was throwing:

org.h2.jdbc.JdbcSQLException: Timeout trying to lock table "DEPARTMENTS"

    I tracked it down to the following:

1. The testcase invokes "delete from departments"
2. TriggerCreateOnce.fire() invokes PreparedStatement.execute(), where the PreparedStatement is for "DELETE FROM permissions WHERE id=?"
   
3. The PreparedStatement invokes ConstraintReferential.existsRow() to ensure that no rows reference "permissions" as a foreign key, but in so doing blocks trying to establish a table lock.

I believe this is caused by the fact that DEPARTMENTS has a ON DELETE CASCADE constraint on the permissions table. The CASCADE tries to lock DEPARTMENTS but the PreparedStatement (running in the system connection) already has it locked. Using the system connections from Trigger.init() lead to very ambiguous and hard-to-debug failures :(

    Anyway, I modified the PreparedStatement to use "SELECT * FROM" instead of "DELETE FROM" to work around this issue.

    So, in conclusion:

• I believe I demonstrated there is a noticeable performance benefit for the Trigger2 interface.
• I believe there is an ease-of-use benefit to steering users away from the use of system connection (due to the aforementioned problems).
  

Action items:

• Can H2 throw a deadlock exception when a system connection and user connection running under the same thread attempt to lock the same table (instead of a "Timeout" exception)?
  
• What are the next steps for Trigger2?
• If we introduce Trigger2.initTransaction()/closeTransaction() is there still a legitimate need for system connections, or can we remove them from init()?
  

Thanks,
Gili

[Gili]

Ping.

Can we at least open an issue to track this? I don't want this to fall through the cracks.

Gili

Regards,

Thomas

To unsubscribe from this group and stop receiving emails from it, send an email to h2-database+unsubscribe@googlegroups.com.

To post to this group, send email to h2-da...@googlegroups.com.
Visit this group at http://groups.google.com/group/h2-database.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--

You received this message because you are subscribed to a topic in the Google Groups "H2 Database" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/h2-database/Sb3T1aVwoCE/unsubscribe.

To unsubscribe from this group and all its topics, send an email to h2-database+unsubscribe@googlegroups.com.

To post to this group, send email to h2-da...@googlegroups.com.
Visit this group at http://groups.google.com/group/h2-database.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
You received this message because you are subscribed to the Google Groups "H2 Database" group.

To unsubscribe from this group and stop receiving emails from it, send an email to h2-database+unsubscribe@googlegroups.com.

To post to this group, send email to h2-da...@googlegroups.com.
Visit this group at http://groups.google.com/group/h2-database.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
You received this message because you are subscribed to a topic in the Google Groups "H2 Database" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/h2-database/Sb3T1aVwoCE/unsubscribe.

To unsubscribe from this group and all its topics, send an email to h2-database+unsubscribe@googlegroups.com.

[Noel Grandin]

Feel free to log it in the issue tracker here:
https://code.google.com/p/h2database/issues/list

[Gili]

Done: https://code.google.com/p/h2database/issues/detail?id=511

Gili