Use savepoints for nested transactions

Do all 3 databases with core support use the same syntax?

And is there a performance benefit here? I just want to be sure it's worth spending time on this issue when core barely uses transactions and there are 340 critical issues remaining in the D7 queue. :-)

To preserve transaction atomicity, the outermost operation would still need to be a real transaction.

As far as PostgreSQL is concerned, savepoints require a real transaction.

So the transaction API when then work something like:

• $txn = db_transaction() is called and BEGIN is sent to the database
• Somewhere further in the code $txn2 = db_transaction() is called again before the previous $txn loses scope, SAVEPOINT savepoint_1 is then sent to the database
• Any number of savepoints can be created with an auto increment suffix (savepoint_1, savepoint_2, so on)
• When it is time to commit the transaction, if it is actually a savepoint, Drupal will remember the last savepoint created and release it. This will re-occur every time a transaction variable loses scope until the inital transaction variable loses scope at which point the transaction COMMITs
• $txn->rollback() will send ROLLBACK TO SAVEPOINT savepoint_n to the database where n is the last savepoint to be created.

So in application:

try {
  $txn = db_transaction(); // BEGIN;
  db_insert('node')
    ->values($values);
    ->execute();
  
  try {
    $txn2 = db_transaction(); // SAVEPOINT savepoint_1;

    db_insert('node')
      ->values($values2);
      ->execute();
    // Commit second transaction.
    unset($txn2); // RELEASE SAVEPOINT savepoint_1;

  } 
  catch (Exception $e) {
    $txn2->rollback(); // ROLLBACK TO SAVEPOINT savepoint_1;
  }

  // Commit first transaction.
  unset($txn); // COMMIT
}
catch (Exception $e) {
  $txn->rollback(); // ROLLBACK;
}

I like this idea, it can make transaction support more similar from database to database. For example, InnoDB can paritally commit failed transactions, using savepoints instead means all three databases can can commit intentional partially successful transactions.

yes, they all use the same syntax - don't know about performance - I defer to David Strauss for that.

Hm. This is outside my knowledge scope, but I'm game to try it if Drieschick is. I'm pretty sure it would have to be in by the 7th, though, and then we'd still need to address the sequence issue and all of the others that depend on it. Gah.

I say go for it. I'll try to prioritize reviewing this chain of patches, as the dependency chain here is getting rather long.

:S only saw this comment now. IMO, I think its a better way to do embedded transactions but its not critical for D7. I say put it in D8.

I'm going to bring this back into play because I think PostgreSQL (and most likely SQLite) needs this for atomicity (#301036: Optimize Merge queries in Postgres driver) here is a starter patch that introduces savepoints rather than false transaction nesting.

Because savepoints require a name, you can now optionally pass a name to db_transaction() which means there is the potential to rollback to a savepoint that wasn't the last savepoint introduced:

$txn1 = db_transaction();
//  do some query stuff.

$txn2 = db_transaction('node_updates');
// do some more query stuff.

$txn3 = db_transaction();
// do even more query stuff.

// $txn2->name will return 'node_updates' but you can use ->name() when you don't know the name.
Database::getConnection()->rollback($txn2->name()); // rollback to $txn2. Queries between $txn1 and $txn2 remain executed.

But the real beauty of this fix means we can now have a query fail but still have a successful transaction:

// simplified version of db_merge
$txn = db_transaction();
try {
    db_insert($table)
        ->fields($fields)
        ->values($values)
        ->execute();
}
catch (Exception $e) {
     $db_transaction->rollback();
    db_update($table)
        ->fields($fields)
        ->values($values)
        ->key($key)
        ->execute();
}

We are atomic here because we're only executing one query according to the transaction.

found spelling mistake already.

Subscribe, looks very promising! I was using savepoints with oracle and there was no performance troubles.

Here are my latest changes, there are still two failures in the transaction test though.

@David Strauss it gives you an option to identify a transaction point, say for example you have several transactions in the same scope, lets call then savepoints A, B anc C where C is the most recent savepoint established. Say for some reason in your code, you need to rollback to savepoint B rather than C you can simple call Database::getConnection()->rollback('B') which will rollback to that savepoint.

Does that make sense? Is it a huge performance problem to remember them? From the logs, there only ever seems to be a depth of 2 at any one time in core.

Let's test last patch...

reroll against current head

@Josh You lost a "}" after generateTemporaryTableName(), so here new reroll

Suppose this fixes are last :)

1) transactionOuterLayer() was forget to make rollback if $rollback parameter passed
2) popTransaction() has no break after releasing savepoint so pops all savepoints

Minimum version 3.6.8 is required for savepoint under sqlite http://www.sqlite.org/changes.html
pgsql works fine!
need testing under mysql without transactions :(

Looking good overall. A few issues from visual inspection:

Perhaps I'm missing it, but transactions start out as optional-named (there is a default of empty string on DatabaseConnection::startTransaction()), but later down in DatabaseConnection::pushTransaction() it's required with no default.

Any new database exception classes should be prefixed with Database. Really, the existing ones should be, too, but they were written before we established that convention and I don't know if Drieschick will let us change them. :-) (If we're changing the transaction system anyway, I'd change the transaction exceptions to be DatabaseTransactionBlahBlahException at least.)

Protected properties on a class should be lowerCamelCase, not underscore_casel. (Eg, DatabaseTransaction::has_rolled_back)

New patch

- renamed all (4) transaction-related exceptions
- has_rolled_back replaced with rolledBack
- a bit changed php-docs

api change

current http://api.drupal.org/api/function/db_transaction/7
db_transaction($required = FALSE, Array $options = array())

new 
db_transaction($name = NULL, $required = FALSE, array $options = array())

I think we should remove $required argument because it useless - patch adds $name argument to database::startTransaction() but there's no $required!

savepoints naming

- savepoints should have unique names
- rollback possible to any savepoint in stack
- api should provide ability to set own name and rollback to any named savepoint

By default first savepoint receive "drupal_transaction" name
Next savepoints get incremental "savepoint_$depth" name
Else developer could define own name

If name already used so DatabaseTransactionNameNonUniqueException will be thrown

PS: Josh, please help with documentation and my english grammar.

docs
http://dev.mysql.com/doc/refman/5.0/en/savepoint.html
http://www.postgresql.org/docs/8.3/static/sql-savepoint.html
http://www.sqlite.org/lang_savepoint.html

Thanks for the work Andy! the patch looks good, test bot likes it. I think the comments in the patch are fine. So we need to put docs somewhere else?

I want Crell to sign-off on this patch.

It looks like the wrapper functions still have the $required parameter, but the methods simply drop it. If my sieve-like memory serves, we decided to remove the required property entirely so we should just get rid of that parameter to the wrapper functions.

The exception "DatabaseTransactionsNoActiveException" seems like a screwy name to me. I don't know if DatabaseTransactionNoActiveTransactionException would be better or worse, though. :-)

Actually, since we've removed the required property, that exception isn't needed. The only place we throw it is in the overridden Database::commit() method, where it's technically wrong anyway.

So let's do this:

1) Remove the $required parameter.
2) Rename DatabaseTransactionsNoActiveException to DatabaseTransactionExplicitCommitNotAllowedException (or something like that), since that's the only time it's used.

I think we should save $required as part of $options there are some modules that could require explicit transactions

I leave 'required' as optional parameter for startTransaction() and a part of $options parameter for db_transaction()
When transactions disabled and $required is TRUE a DatabaseTransactionsNotSupportedException exception will be thrown.

About exceptions:

DatabaseTransactionsNoActiveException already have nice description and make no sense for commit() - thrown when trying to rollback but there's nothing to rollback!

For commit() now used DatabaseTransactionExplicitCommitNotAllowedException exception

@Crell Please point why $required should be totally dropped?

As I found #616650: Default MySQL to transactions ON just removes a $required parameter but having this with FALSE much reasonable.
Because this could be helpful with modules which require transactions!

@andypost: Because the functionality of $required was already removed. See HEAD right now; it looks like the issue you linked simply missed some bits of it, and what's left is unused flotsam that we should remove anyway since $required does nothing to begin with. If it doesn't get removed here, we'll just need to clean it up in another patch.

And no, the use of DatabaseTransactionsNoActiveException exception is incorrect. It means, per the name, "you're trying to roll back and there is nothing to roll back". What the commit() override method is saying is "dude, you're not allowed to roll back transactions that way, use the other way we gave you." Those are not the same error message.

@Crell so in last patch - 2 different exceptions - exactly as you said:
DatabaseTransactionsNoActiveException - nothing to rollback
DatabaseTransactionExplicitCommitNotAllowedException - commit is not allowed by this way

About $required - do you really think that core does not needed this functionality at all?

We could save this at DB API as core rely on old-users-with-myisam-tables... Optional parameter is not hard to save.

Yes, I can't actually think of a case where a *module* would absolutely require transactions. I can think of use cases for sites, but that's up to the site/DB admin to setup MySQL properly with InnoDB. I actually rather like David's idea of pushing that check to the module's info file instead of we really really need to have it.

Ah, looking at the patch now I see the separate exception classes, thanks.

We don't actually need to do anything, it's easy enough for a module to implement a hook_requirements() that checks:

Database::getConnection('default')->supportsTransactions()

Let's just remove that completely.

Ahso, right you are! OK, so we already support module-level "I really need it" checking, so there's no need to have it baked into the API. Let's finish removing that and then get the rest of this patch in.

This patch removes $required and it's exception also unified doc-blocks

Now patch against current HEAD

From Crell:
@joshwaihi When I last looked at it I think it was pretty good. Now that the $required stuff is fully gone, we're probably good to go.

Since I don't think twitter is the best venue for a code review, consider this a +1 let's commit things thing. :-) 4 of the 5 DB team members have been in this thread and all support it, so let's get 'er done.

Re-roll after #711682: Style issues in includes/database/database.inc

Also updateв doc-block for DatabaseConnection::rollback() to point using 'drupal_transaction' as name

   * @param $savepoint_name
   *   The name of the savepoint or transaction. Use 'drupal_transaction' to
   *   rollback all started transactions.

We could make this parameter optional and check for !isset() and assign this default

Doc review of the above patch... For reference, here is the guidelines page: http://drupal.org/node/1354

a)

-   * @var int
+   * @var array

I don't think the API module supports the @var tag at all -- should this even be there?

b)

+  /**
+   * Determines current transaction depth
+   */
+  public function transactionDepth() {

First line of doc header should end with a period.

c)

+   * @param $name
+   *   Optional name of the savepoint.
+   *

I don't think "savepoint" is a word (unless it is some transaction jargon I might not be familiar with)... Maybe "save point" or "transaction save point" or just "transaction" would be clearer? I am willing to be told I'm wrong on this, if you can point to where in official PHP or MySQL doc they use the term "savepoint" for this, as I'm not a transaction expert.

d)

   /**
-   * Schedules the current transaction for rollback.
+   * Rolls back whole transaction or to savepoint defined by $savepoint_name.
    *
    * This method throws an exception if no transaction is active.
    *
+   * @param $savepoint_name
+   *   The name of the savepoint or transaction. Use 'drupal_transaction' to
+   *   rollback all started transactions.

First line is awkward. In @param, verb should be "roll back" not "rollback". Suggestion for first line:
Rolls a transaction or save point back.

e)

+  protected function logRollback() {

This function, after your patch, has no doc block. It should, even if it's just a one-liner.

f)

+/**
+ * Exception thrown when commit() function fails.
+ */
+class DatabaseTransactionCommitFailedException extends Exception { }

Needs "a" or "the" in there, like "thrown when a commit()".

g)

+ * @param array $options
+ *   An array of options to control how the transaction operates.
+ *     - 'target' The database target name.

Indentation is incorrect. Check the Lists section of the guidelines page above.

a) used everywhere to describe class members
b) fixed
c) savepoint is a term (details at comment #24)
d) changed a bit
e-g) fixed

Suppose we could fix docs with follow-up patch

g) changed - no quotes

The problem with fixing docs in follow-up patches is that no one is interested in doing the follow-up patches, so we end up with a zillion issues in the documentation queue and no one interested in fixing them.

It is best instead never to commit code that violates our coding/doc standards, in my opinion.

So thanks for making the fixes!

One more in the latest patch:

+   * Logs a failed rollbacks.
    */

No S on rollback.

@jhodgdon should be rollbacks - plural, because it could write a set of operations. Also I'm not good with English to write good comments so glad any help!

If it should be rollbacks (plural), then the "a" should be removed. The comment should say "Logs failed rollbacks."

in plural you don't need to write the a.

==> Logs failed rollbacks

or

logs the failed rollbakcs

jhodgdon will decide ;)

Ummmm....

It looks to me as though what this function does is:
a) Logs that the rollback failed.
b) Logs each stored error in the rollback... though I am not really sure what this means since I have not read the whole transaction/rollback code base...

  protected function logRollback() {
    $logging = Database::getLoggingCallback();
    // If there is no callback defined. We can't do anything.
    if (!is_array($logging)) {
      return;
   }

    $logging_callback = $logging['callback'];

    // Log the failed rollback.
    $logging_callback('database', 'Explicit rollback failed: not supported on active connection.', array(), $logging['error_severity']);

    // Play back the logged errors to the specified logging callback post-
    // rollback.
    foreach ($this->rollbackLogs as $log_item) {
      $logging_callback($log_item['type'], $log_item['message'], $log_item['variables'], $log_item['severity'], $log_item['link']);
    }

    // Reset the error logs.
    $this->rollbackLogs = array();
 }

So I think the doc header should explain that? It looks like it is logging the rollback of one transaction, so if it's not going to explain about the playback (whatever that is), I would vote for

"Logs a failed rollback."

Patch with "Logs failed rollbacks." thanks for explanation!

@jhodgdon This patch brings possibility to use nested transactions. Every nested transaction has it's own 'savepoint'. A mess caused by using words transaction and savepoint because transaction is a wrapper for set of savepoints!

About logger: suppose in the most of cases the $this->rollbackLogs array will hold a one record but sometimes could hold more records. It's all because now transaction have more then one level of nesting.

* removed $willRollback
* made drupal_transaction the default name of the transaction
* made $savepoint_name optional in DatabaseConnection::rollback. Will rollback entire transaction if no parameter given.
* updated documentation

whoops, hunk from another patch got in there.

I think now it's more clear. All changes reasonable and discussed at IRC.

Let's wait final word of Crell :)

There are still some comments in this patch that need some work, to conform with our coding/doc standards:

a)

+        // If it is the last the transaction in the stack, then it is not a
+        // savepoint, it is the transaction itself so we will need to rollback
+        // the transaction rather than a savepoint.

b)

+   * Logs failed rollbacks.
    */
+  protected function logRollback() {

I think this function logs a single rollback, so the doc should be "Logs a failed rollback.". Either that or the function should be renamed to logFailedRollbacks() [with an S].

c)

+    // Commit everything since SAVEPOINT $name

All in-code comments are supposed to end with a .

d)

+  /**
+   * Retrives the name of the transaction or savepoint.
+   */

Retrives -> Retrieves

e)

+ * @param array $options
+ *   An array of options to control how the transaction operates.
+ *   - target: The database target name.

Needs operates: at the end of the line, not operates.

Fixed all
about a) - suppose "need to roll back" on a second line

Changed descriptions of logRollback() to
Logs a rollback() messages.

Seems more clear that it logs any messages that stored in $this->rollbackLogs array while rollback() called for nested levels of the transaction.

This array accumulates messages because they could not be written to logger (mostly in DB) until the transaction completes.

Grammar: You can't say "logs a" with "messages". It needs to be either "logs a message" or "logs messages".

Actually, I think in this case it should be "Logs rollback messages." or "Logs messages from rollback()." And we can hope that the API module will use this to generate a link to the rollback() method on the same class.

Also just noticed a spelling error: propogate -> propagate

I choose "Logs messages from rollback()."
Also both "propagate" fixed in tests.

EDIT: API module does not handle classes so links is not possible

The API module is working on handling classes.

I have no further objections to the comments in this patch. Will leave the code review to others.

Code was already reviewed, so back to RTBC

Crell confirmed this patch at #41
At #52 Josh has cleaned some unused variable and made default parameter value

Just chasing head.
While browsing through the critical issue list I noticed that this one is important to fix some other issues.
So it's best to get this in soon :)

Hate windows endings

Looks great, and we got sign-off from Crell per my request. Committed to CVS HEAD.

Just tried to install sqlite and get the error "SQLSTATE[HY000]: General error: 1 near "SAVEPOINT": syntax error"

http://img.skitch.com/20100325-x9wckdjebt4e32h82jj481f5gc.png

@mfer could you look at docs #22 and #24

Minimum sqlite version 3.6.8 is required for savepoint support http://www.sqlite.org/changes.html

Last patch does not take into account changes from #748982: Allow Database logging_callback to be a class method

Follow up patch to fix #748982: Allow Database logging_callback to be a class method

Looks good, thanks Andy!

@andypost This creates an interesting problem. 3.6.8 is not shipped with many versions of PHP 5.2. So, while Drupal 7 is stated to require php 5.2 the sqlite version that ships with php 5.2 is not new enough. Isn't that a contradiction? It will be confusing for many PHP 5.2 users and difficult for people to test.

Currently sqlite support blows up under tests. Yet, MAMP doesn't even come with sqlite 3.6.8 so it's not easy to ask people to test to help get it all working.

@mfer I think sqlite maintainers could help with this. I know about this but I think using sqlite is limited cases so it require some docs how to build php with required sqlite version.

Also I see no reason of using sqlite in multiuser environment.

EDIT: suppose sqlite could provide it's own overriden method for transactions

@andypost You can't build your own version of SQLite on shared hosting. Or, on managed VPS. Or, if you aren't at all a server admin. Because, then you need to maintain your own custom build which is another step as well.

Requiring a newer version of SQLite than what ships with PHP 5.2 severely cripples its usage and the people willing to develop for it.

FYI, the first version of PHP to contain SQLite 3.6.XX is PHP 5.3. See http://www.php.net/ChangeLog-5.php#5.3.0

So, for SQLite support someone either needs PHP 5.3 or a custom version of SQLite compiled in.

I agree that we need to support "out of the box" SQLite in 5.2. That probably means an SQLite-specific transaction implementation that is more like the previous implementation that just fakes savepoints and rolls back everything.

Ugh.

I'm marking this needs work. Right now you cannot test Drupal 7 in SQLite with PHP 5.2. This is a problem :)

The RTBC is for the patch in #70, which shouldn't be held up by the SQLite issue. Once that fix is committed, this can go back to "needs work".

I'm getting tired of big patches like this one breaking support for SQLite.

Let's file another issue about sqlite support and close this after commit.

@damian, if all db enviroments were tested on the testbeds this wouldn't be an issue. Didn't Dries commit #70?

I did commit #70.

I guess this means that status is 'needs work' instead.

@Dries follow up still not commited!

Confirm that current ubuntu ships with SQLite 3.6.16 - and it works!

@andypost Your argument that ubuntu shipping with a different version of SQLite than the default version shipped with php 5.2 will not work. This needs to work for the default PHP 5.2 or we need to change the system requirements required to run drupal.

@mfer I've changed status to point that Dries forget to commit #70

Let's revert #65 then do this without breaking sqlite support or not at all, it's too late in the game to be breaking major features then deferring fixing them to followup patches.

@catch I think it's better to make patch for sqlite than break nested transactions.

Also this approach is needed to finish #715108: Make Merge queries more consistent and robust

I file a critical issue #754192: Transaction support for old sqlite versions
Suppose it's much easy to check version of sqlite and fall back to limited transaction support for single-user database then stay without atomic operations for the most common databases.

So let's commit this follow up and cotinue in #754192: Transaction support for old sqlite versions

+1 to #91. Let's commit that and deal with SQLite in the other issue so that we don't continue to block other issues.

I committed the patch in #91. I thought I had committed it because it looked a lot like a watchdog patch that I had committed earlier in the day. Sorry about that.

I'm not inclined to rollback this patch entirely. It sounds like we might be able to make SQLite work by simply overwriting a few functions with empty stubs so that transactions don't do anything useful.

There's already another issue for dealing with SQLite transactions linked above, so this should be marked fixed.

#754192: Transaction support for old sqlite versions is a very important issue. D7 currently fails A LOT of tests in SQLite. Until that patch lands we cannot test SQLite. This could be a mean a RC before Drupalcon either FAILS tests in SQLite (which means there are bugs) or doesn't come out.

I'm seeing lots of new errors: "Explicit rollback failed: not supported on active connection.". I think this logRollback method is getting called even for successful rollbacks which appears to be unintended. I can't work out the real intentions from looking at the code. Anyway, here is a patch which seems to work. If we really do want to call this method unconditionally, then it shouldn't always report a failed rollback.

Hopefully this properly repairs the minor butchering added in this issue.

@moshe weitzman This change required for non transactional Dbs. So sqlite should be fixed too because it mirrors this logic #754192: Transaction support for old sqlite versions

@David Strauss: So do you think that we should re-open #754192: Transaction support for old sqlite versions and remove all checks for transactions? I think we should keep database.inc in-sync

IMO, its fine for this code to mirror as andypost has suggested. Thats not really the intent of this patch though. We're fixing a critical new bug in HEAD.

Committed to CVS HEAD. Thanks.

Committed to CVS HEAD. Thanks.