Nested transactions throw exceptions when they got out of scope

Should we check in_transaction state when calling DDL?

This is a known issue. On MySQL, DDL is non transactional. (MySQL is the last database engine, with Oracle that has non-transactional DDL)

I vote for simply documenting that.

As a side note: all our DML is now transaction-safe, including TruncateQuery. This is more then what, for example, Ruby on Rails Active Record can say.

Subscribing.

I think this is a problem that has to be dealt with other than by just documenting it. As far as I know, there is no way to tell whether you currently are in a transaction or not, so with this bug every function that executes DDL statements would have to know whether it may be called from within a transaction, or not. Since other modules might use functions of your module, you would essentially also have to document which functions may be called from within an transaction — and that recursively, if you think this to the end.

Since the error during RELEASE SAVEPOINT doesn't actually do any harm (since the transaction was committed, as intended), I'm all for just catching this exception. It's a lot more complicated when trying to roll back a change, though. In that case, throwing the exception might be the best option, even though that also doesn't solve the problem, i.e., that the rollback essentially doesn't work in this case, making the whole transaction ineffectual.

that's weird as transactions would make sense in particular for configuration-like stuff like node-types (what is impossible with that restriction).

That restriction also means it is impossible to execute DDL statements triggered upon those transaction-safe hooks, e.g. you cannot write a module that creates a field somewhere triggered by a node creation.

I just ran into another problem with nested transactions (with mysql). Maybe this is somehow related?

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: RELEASE SAVEPOINT savepoint_1; Array ( ) in variable_set() (line 805 of /var/www/drupal-7/includes/bootstrap.inc).

variable_set() makes use of a merge query which makes use of a transaction - so maybe there is general problem with nested transactions?

I'm not sure if this is a general problem or not. From the sound of it, checking for "no need to commit this transaction since it already happened" seems like a worthwhile thing to do; rolling back the transaction after it's been committed we should probably let throw an exception.

IMO, if you want to commit a transaction that's already committed then the post-condition is the same either way; the transaction is committed by the time you wanted it to be. We took the same position on deleting indexes and tables and such in the schema API so I think it's OK to "silently succeed" here.

In my opinion, this should rather be in the database-specific code for those databases that need it. So, DatabaseConnection::popTransaction(), around the RELEASE SAVEPOINT query would be a good place.

But in principle this patch works great for me, thanks!

This will need unit tests, too.

If we want to ignore the error here, we also need to make sure that the pushTransaction() that follows the failed popTransaction() starts a new transaction.

I can confirm that the patch in #11 fixes my problem described in #9 too.

This will need unit tests, too.

Right.

This exception catching definitely belong in popTransaction() and also need to reset the transaction depth, as mentioned in #16.

Oh, yes, sorry — the test was meant more as a starting point, I know that there are still issues with this approach. Should have been a bit less tight-lipped.

However, it's at least good to see that this test works correctly.

OK, so how about this?
No real expert in the database stuff, but it works and also seems like what you suggested.

+++ includes/database/database.inc	13 Jan 2011 22:06:41 -0000
@@ -1030,7 +1030,22 @@ abstract class DatabaseConnection extend
+          // If one SAVEPOINT was released automatically, then all were.
+          // Therefore, we keep just the uppest transaction.
+          $this->transactionLayers = array('drupal_transaction');

"uppest" is not a word. I think you mean "top most".

Powered by Dreditor.

If this was really the only thing wrong with this patch, can we please set this to RTBC? It's a really annoying issue, that can easily be fixed, as it seems.

#22: 1007830-22.patch queued for re-testing.

It can hardly be the case that changing one word in a comment makes 3 tests fail …

@ bfroehle: I'm no expert, but I think that this is not the case. The topmost transaction is handled with a "real" database transaction, not with a SAVEPOINT, which means that it won't be touched by this bug. We just have to keep track internally of the state the database is in, i.e., just the topmost "real-transaction" transaction is active.

#25: 1007830-25.patch queued for re-testing.

OK, the old patch still passes, so there should be no way the new one fails, right?

I thought that too (also after reading some docs), but apparently (and as the passing tests indicate, the test bot agrees with me there) only the savepoints get released, while the MySQL transaction is able to handle the DDL statements.
As you see, only the case where a SAVEPOINT is released is in the TRY … CATCH, not the case where the database-level transaction is committed. Also you can try it out yourself – having just a single (not nested) transaction doesn't trigger the error. At least not for me …

We are prototyping InfiniDB with a D7 install and were running into the nested transactions error on DDL operations from installing modules - so seeing similar errors as were reported on a more typical InnoDB installation.

Applying the patch to our installation appears to get us around the issue which opens the way to running D7 on InfiniDB for more integrated analytics applications. TY for the patch - cheers.

I just ran over this again - see #1047700: Fatal error when trying to delete a profile type which contains fields.

Given the explanation in #31, the patch makes sense to me.

ok, talked to damz about this. As the problem is mysql specific, we should fix it in the mysql-specific driver. So I re-rolled the patch to do so. For that I had to invent commitTransaction(), a method allowing db-drivers to directly commit the transaction.

Subscribing.

+  protected function commitTransaction() {
+    return parent::commit();
+  }

It's little known, but in PHP you can skip some level of inheritance. You can just use:

PDO::commit();

... instead of adding a new method.

But that's a static call then? PHP issues E_STRICT warnings for calling non-static methods statically.

Oh thanks for pointing that out. Just learned something new :)

I am not sure if this error belongs in this thread but google led me here and it seems to fit.

I am running Profile 2 on Drupal 7 and went to admin/structure/profiles and deleted a profile. This is the error that I got.

PDOException: SQLSTATE[HY000]: General error: 1305 SAVEPOINT savepoint_1 does not exist in field_read_instances() (line 706 of C:\Users\local\Desktop\website\modules\field\field.crud.inc).

Is this the place to post this? Is there a specific (correct) procedure for deleting a profile?

subscribing

And does the patch in #39 solve the problem? At first glance, this seems to be the same problem, yes.

Same question to Oceanman. Although his exception looks kinda different – maybe a slighty different system setup? Or the error is unrelated, which could be the case, too.

subscribing

Seeing this via profile2.module :-\

I don't pretend to understand what this patch does at all, but I've tried it and can confirm it fixes the problem with Profile2 module (simplest way to reproduce it: #1047700: Fatal error when trying to delete a profile type which contains fields).

Here is it rerolled without the whitespace changes.

Upping to major -- this causes fatals in several contrib modules.

I also tested it and can confirm it fixes the problem with Profile2 module.

Needs to go in 8.x first, otherwise looks good.

Would like to see Crell sign of on this.

*signs his name on the dotted line*

tagging for backport.

Shouldn't that then need these status changes too? :)

No, because it hasn't been committed to 8.x yet. The patch applies to 8.x with offset, so it should be possible to commit the same patch to both branches.

As this repeatedly causes fatals for people and urges modules (like the entity API) to do weird workarounds to minimize the effects, this really needs to go in before 7.1 ships.

Note: I reviewed #47 and can confirm it's identical to #39 beside the obsolete whitespace-fix.

#47: 1007830.drupal.nested-transactions-ddl.patch queued for re-testing.

I've seen this SAVEPOINT_1 errors a lot. I'm working with Feeds, entity API, search API. Please put that fix in 7.1

+1 for 7.1

Thanks for the detailed discussion here.

MySQL is doing a weird thing here with committing transactions too early if they involve certain operations such as ALTER or CREATE table statements. When it attempts to release the transaction BOOM things explode. This is causing fatal errors for contributed modules such as Profile2.

The solution is to special-case MySQL's driver to silently succeed in this instance. It does make an API change (an extra $ddl_statement argument to transactionInnerLayer()/transactionOuterLayer()), but this API change is backwards-compatible, because it defaults to FALSE.

This approach has sign-off from two database subsystem maintainers (Crell and DamZ), and also comes with unit tests to prove it works. Sounds good to me!

Side note: This is one of those issues where it would make total sense for webchick to be able to commit this patch to both D8 and D7. :-)

Especially when it's been RTBC for nearly six weeks, blocks contrib modules, and missed the deadline for a point release despite all this.

Dries voiced a concern around this patch the other day when we spoke (I think it was mainly due to the size/placement of the patch in relation to how close a 7.1/7.2 release was), so took a closer look today, since my last look was pretty cursory because it sounded like this was all but signed off.

Here's a patch that addresses these comments (marking back to "needs review"):

+++ b/includes/database/mysql/database.inc
@@ -133,6 +133,48 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+      if ($savepoint != $name) continue;

The parent function writes this as:

    if ($savepoint != $name) {
      continue;
    }

...we should do the same here.

+++ b/includes/database/mysql/database.inc
@@ -133,6 +133,48 @@ class DatabaseConnection_mysql extends DatabaseConnection {
+        // As MySQL does not support transactional DDL previous queries may have
+        // implicitly committed pending transactions. To avoid exceptions when
+        // no actual error has occurred, we silently succeed for PDOExceptions
+        // with error code 42000 ("Syntax error or access rule violation").
+        try {
+          $this->query('RELEASE SAVEPOINT ' . $name);
+        }
+        catch (PDOException $e) {
+          if ($e->getCode() != '42000') {
+            throw $e;
+          }

This comment is technically correct, but it took me several reads to understand what it was saying (and even the author of the patch in #47 acknowledges not understanding what's happening). I think broefle did a better job of explaining the issue in the OP, so I've tweaked those surrounding comments to provide more background from this issue for the benefit of future non-DBTNG maintainers. :)

But I have a question. Wouldn't there be other realistic circumstances through which you could raise a 42000? For example, if you misspelled the name of a table? Won't this patch therefore make debugging these types of queries on MySQL much harder going forward, or cause cascading fatal errors or the like?

Oops. Forgot to include the first part. ;)

Webchick's revisions in #63 make sense to me. Although this means we still need Dries to commit this. :-(

To the possibility of this error getting thrown at other times, well, probably. This is, strictly speaking, a hack around the shortcomings of MySQL. I don't know that it gives us enough data to differentiate between different types of 42000. If someone has a suggestion on how to better differentiate the type of error I'm open to suggestions.

Ok, well given that this is causing very real, disastrous problems in contrib right now, and given that the concern raised in #62 is currently a theoretical rather than an actual one, AND given that we just got done with a release so have some time yet to test this and/or come up with a better approach, I think it's better to put this in sooner than later. (I don't think this is violating any rules; my patch was merely some whitespace/comment adjustment to OPP (other peoples' patches), which I sometimes do before committing stuff.)

So, committed to 8.x and 7.x. Thanks a lot, folks!

bfroehle, that's an interesting idea. Although from that page wouldn't it be MySQL code 1402?

Could you post a follow-up patch to try to see if that would work?

Because I misread the page you linked to, that's why. :-)

Patch looks great, but the documentation should note that the 1402 error is a MySQL-specific error code rather than the PDO-"standardized" error code. Let's clarify that in the comment and then this is good to go.

Looks good to me and the bot seems to like it.

I never knew about errorInfo. Nicely done! (ie. yes, let's do this)

Great; thanks for the follow-up. This more targeted approach indeed seems better, and the code got simplified a bit in the process!

Committed to 8.x and 7.x! Thanks!

Hmmm, I'm using the latest drupal dev and am still getting this error:

PDOException: SQLSTATE[HY000]: General error: 1305 SAVEPOINT savepoint_1 does not exist: RELEASE SAVEPOINT savepoint_1; Array ( ) in EntityAPIController->save() (line 694 of C:\Users\Paul\Sites\karaoke\sites\all\modules\entity\includes\entity.controller.inc)

I get it when I save an index in Search API.

Are we sure this issue is fixed? I pressed "back" on my browser and saved again and it worked without an error. From what I've read elsewhere, the error occurs rather randomly and is difficult to reproduce.

I pressed "back" on my browser and saved again and it worked without an error. From what I've read elsewhere, the error occurs rather randomly and is difficult to reproduce.

At least in the Search API, it isn't random but completely reproducible. It will occur for indexes lying on, or being moved to/from a database search server, when changing the indexed fields.
The reason you don't get the error when saving a second time is that the changes are, in fact, correctly stored the first time, even though there is an exception. Therefore, the second save won't actually change any values, and therefore won't trigger the error.

Since this is MySQL specific anyway, perhaps we should drop the SQLSTATE checking (since it seems to be ambiguous)?

+1
Pity we didn't spot this before … Looking at the manual it seems to me there is really no way that this error number could be caused by something other than a missing savepoint with the statement "RELEASE SAVEPOINT sp".

Also, even with that patch there are issues in how we manage the transaction stack, that will hopefully be fixed by #1185780: Make transactions more flexible and useful.

@bfroehle
Using mysql 5.1.54

@drunken monkey
Ahhh, that makes sense. Thanks for the explanation!

Patch looks good.

@ paulgemini: Could you verify that the patch solves the issue for you, too? Please set the issue status to "reviewed & tested by the community" if it does.

Yep, seems to work.

Which patch should be used, bit confusing... apply this path, remove this patch.
Thought it was commited in Drupal build 7.4 and did an update lol.

The main patch is committed, this is still open for a minor followup, you should be able to live without it unless you're still running into problems.

Ok, hopefully this is the last follow-up this time. ;)

Committed and pushed to 8.x and 7.x. Thanks!

I really don't want to re-open this, but I just managed to reproduce this both locally and on the test bot with a reasonably innocent patch at #687180: Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content. The exception is being thrown from execute() during field_read_fields().

If this should go in a new issue, that's fine with me, it's late here so I have not debugged or anything.

Any proper fix would require #1185780: Make transactions more flexible and useful first. The problem here is that we are just resetting the transaction stack, so any remaining open transaction object might freak out when it gets destructed.

Yep, the issue still appears :(
See #1236814: entity-deletion does not make use of transactions

Update: Entity API tests work, however with Profile2 an error can be still re-produced. see #1047700-8: Fatal error when trying to delete a profile type which contains fields

DatabaseTransactionNoActiveException: in DatabaseConnection->popTransaction() (line 1100 of /home/user/web/drupal-7/includes/database/database.inc).

I'm getting DatabaseTransactionNoActiveException: in DatabaseConnection->rollback() (line 1003 of /projects/rtimport/rtimport/www/includes/database/database.inc). on both Migrate rollbacks and VBO mass deletions. Hmm.

Also, I'm pretty sure DatabaseTransactionNoActiveException is misnamed, thus: #1247044: DatabaseTransactionNoActiveException is poorly named

re: #94 Actually, it looks like #87 or #1185780: Make transactions more flexible and useful fixed my problem; my install wasn't up to latest, and when I pulled and updated, I had no troubles. Yay!

Unfortunately, I can still reproduce #1047700-8: Fatal error when trying to delete a profile type which contains fields - so this issue is still not completely fixed :(

I am about 6 hours into developing a new D7 site and I am getting the following errors when I try to add new fields:

There was a problem creating field Test2: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist.

I first got this after installing commerce modules and then attempting to add a field to a taxonomy vocabulary. The worst part about this for me is that now I can't seem to add any fields anywhere without getting this error. Not good. If this is a different bug, my apologies... I'm new to D7 development.

BTW, I was on 7.7 originally and upgraded to 7.8 and that did not fix the issue. I got a clean install of 7.8 and added the same vocabulary and field successfully.

Any ideas on how I can add a field again?

I get this notice "There was a problem creating field Test: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist." after deleting a taxonomy term reference which references a populated taxonomy vocabuary. Subsequently, if I attempt to save a new field of any type to any content type I get the same notice, along with the notice "Your settings have been saved". These fields (saved after the initial error was thrown) do not appear through the admin/structure/types/manage/%/fields UI, although database tables are being created for the fields.

Sorry for the extra noise on the channel - I'm not sure if this is reproducible by simply deleting a term reference field (using autocomplete text area widgit, in my case) referencing a populated taxonomy vocabulary, or is the result of my own meddling.

I would love to find a way to roll the transaction back to a point where I can rebuild my taxonomies (if necessary), add fields to reference them, and get this site launched :) .

Thanks for any help.

This is because we have a wrong assumption in the database layer. We assume that it is wrong to try to commit a non-existing transaction, and right to try to rollback a non-existing transaction.

The real answer is that none are right:

- If we try to commit a non-existing transaction, it's because it has been previously committed (for example by an implicit commit during a DDL statement) or rolled back out of order (a parent transaction has been rolled back). In the first case, we can and should fail silently. In the second case, we already trigger an exception during the rollback.
- If we try to rollback a non-existing transaction, we should raise an exception, because an action should have taken place, but haven't.

As a conclusion:

- Database::popTransaction() should fail silently: either the transaction has already been committed, or an out-of-order rollback has taken place and an exception has already been thrown in Database::rollback()
- Database::rollback() should trigger an exception if trying to rollback an existing transaction

In other words, we need to precisely do the opposite of the current logic. See attached patch.

Also... the new transaction tests where disabled. Oups.

Any idea what a backport of #99 might look like?

Haven't looked at this properly yet, but I quickly ran tests locally with #687180: Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content, no change there though.

Still struggling with this. Any word on a D7 backport? Also, is there any way to track this down manually and resolve the savepoint issue?

EDIT: Really, I need to figure out how to roll back the transaction that is looking for a non-existant 'savepoint_1'. I'm not sure how to do this..

I tried the patch of #99 and it finally solves the profile2 problem described at #1047700-8: Fatal error when trying to delete a profile type which contains fields!!
Also, the description makes sense to me and the patch looks good.

Looking at #101 it seems there are more problems left though :(

I double checked #101 against the testbot in #687180-39: Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content. It does not fix things there but this should not necessarily hold this up either.

Just an update to this, I also get the error:

There was a problem creating field Subscription start date: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist.
Your settings have been saved.

When adding a new field.

Drupal 7.8, using the Features module for fields.

I tried using the D7-dev database files for /includes but same error.

Bizarrely, the workaround is to disable the overlay ( remove overlay part from URL ) then I'm able to add new fields every time.

So yes, disabling the overlay solves this error everytime.

I hope that helps someone else too.

cheers,

DT

#99: 1007830-rollback-commit-non-existing-transactions.patch queued for re-testing.

Analyzing the other issue, this still fails:

$main_transaction = db_transaction();

$transaction = db_transaction();

// Do something DDL.
db_query('TRUNCATE cache_page');

unset($transaction);

$query = db_query('SELECT * FROM variable');

foreach ($query as $row) {
  $transaction = db_transaction();
  unset($transaction);
}

The last issue here was that our DDL transactions tests were absolutely not testing what they should be testing, so I rewrote them.

When you issue a DDL statement, MySQL implicitly commits the transaction, so we should reset the whole transaction stack and tell PDO that the transaction is over.

The new DDL transaction tests also highlighted that SQLite was incorrectly marked as not supporting DDL statements. Fixed in there too.

The only remaining issue (marked as a TODO in this patch because I do not think we need to fix it right now) is that trying to rollback a transaction containing DDL statements will fail silently. The cause is deep in the stack: if you issue a ROLLBACK statement to MySQL, it will fail silently if no transaction is active. The only option we have to properly fix that would be to track the DDL statements we issue to MySQL (basically everything we do in DatabaseSchema) and invalidate the transaction ourselves. That's not a change I think is in the scope of this patch.

Patch for D7.

Cheers Damien Tournoud, #108 worked great for me.

I had a a PDO error 'PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist in field_read_instances() (line 703 of /modules/field/field.crud.inc).'

This occurred when updating fields for Profiles2 with Drupal 7.9 recommended. This also occurred when creating commerce product types.

All seem seems fixed now, thanks.

Try this patch again (D7)

When adding/editing a date field on D7, I got the same error as #113.
The patch from #116 fixed the issue.

Great to hear. Can we consider this RTBC?

Yep, straight re-roll, RTBC.

Er webchick points out this never got into D8, so moving back to there, but I thought I'd already committed this apparently so it is likely ready.

Re-rolling for D8 with updated file positions and offsets (for database.inc and database_test.test).

#121: rollback_commit_non_existing_transactions-1007830-121.patch queued for re-testing.

#121: rollback_commit_non_existing_transactions-1007830-121.patch queued for re-testing.

Ran into this problem in D7.8 every time I tried to add a Taxonomy field. Patch from #116 fixed it for me.

Will commit this soon if no objections.

Committed/pushed to 8.x, there's already 7.x patches here, so leaving RTBC.

#116 for D7 is ready to roll

Ok! :D Thanks for the expanded tests folks. Let's hope 8th time's a charm with this nasty little bug. ;)

I'm not 100% sure this is the same bug(or a bug in commerce), but sure looks like it.

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: ROLLBACK TO SAVEPOINT savepoint_1; Array ( ) funktiossa CommercePaymentTransactionEntityController->save() (rivi 113 tiedostossa /services/share/git/xxxxxx/web/sites/all/modules/contrib/commerce/modules/payment/includes/commerce_payment_transaction.controller.inc).

This starts to happen at some point when we run JMeter tests, so it sounds like a race condition of some sort.

@bfr: Make sure you are using Drupal 7.12 or higher. Otherwise file a new issue please.

@Dave, per #132 that sounds like you mean if you are using higher than 7.12 then to reopen this post if the error is still persisting. So, in the spirit of that....

I am getting this error with 7.15. They are consistently happening in pairs.

php

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: ROLLBACK TO SAVEPOINT savepoint_1; Array ( ) in file_usage_add() (line 661 of /var/www/buyagainbaby.com/includes/file.inc).

node

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: ROLLBACK TO SAVEPOINT savepoint_1; Array ( ) in file_usage_add() (line 661 of /var/www/buyagainbaby.com/includes/file.inc).

Hope this helps. I ll open a new issue if need be.

Yes, please open a new issue.

New issue added as requested, in case anyone is still getting this error and needs to post about it.

http://drupal.org/node/1803886