In MySQL (InnoDB), savepoints are committed when tables are altered or created. This can cause exceptions when the DatabaseTransaction class attempts to release savepoints which no longer exist.

For example, consider

<?php
function break_it() {
 
$transaction = db_transaction();
 
$schema = array('fields' => array('i' => array('type' => 'int')));
 
db_create_table('breakit', $schema);
}

$txn = db_transaction();
break_it();
?>

If I run this code, say using drush scr test.php, the following error is reported:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist: RELEASE SAVEPOINT savepoint_1; Array
(
)
in break_it() (line 21 of /Applications/MAMP/htdocs/drupal/test.php).

For a discussion of how this can arise, see the related issues:
#776222: Add transactions to more _save() functions
#995820: Exception sometimes when saving
#1004332: db_set_active() commits transactions

Files: 
CommentFileSizeAuthor
#121 rollback_commit_non_existing_transactions-1007830-121.patch10.79 KBbasic
PASSED: [[SimpleTest]]: [MySQL] 34,394 pass(es).
[ View ]
#116 rollback_commit_non_existing_transactions-1007830-115.patch10.71 KBbasic
PASSED: [[SimpleTest]]: [MySQL] 37,359 pass(es).
[ View ]
#109 1007830-rollback-commit-non-existing-transactions7.patch10.94 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1007830-rollback-commit-non-existing-transactions7.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#108 1007830-rollback-commit-non-existing-transactions8.patch11.02 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 33,772 pass(es).
[ View ]
#99 1007830-rollback-commit-non-existing-transactions.patch3.95 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#80 1007830-follow-up-Remove-unnecessary-SQLSTATE-.patch1.4 KBbfroehle
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1007830-follow-up-Remove-unnecessary-SQLSTATE-.patch.
[ View ]
#70 1007830-follow-up-add-mysql.patch1.72 KBbfroehle
PASSED: [[SimpleTest]]: [MySQL] 29,913 pass(es).
[ View ]
#68 1007830-follow-up.patch1.72 KBbfroehle
PASSED: [[SimpleTest]]: [MySQL] 29,905 pass(es).
[ View ]
#63 1007830.drupal.nested-transactions-ddl.patch6.97 KBwebchick
PASSED: [[SimpleTest]]: [MySQL] 29,879 pass(es).
[ View ]
#62 1007830.drupal.nested-transactions-ddl.patch6.95 KBwebchick
PASSED: [[SimpleTest]]: [MySQL] 29,904 pass(es).
[ View ]
#47 1007830.drupal.nested-transactions-ddl.patch6.71 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 29,764 pass(es).
[ View ]
#39 drupal_db_transaction.patch7.55 KBfago
PASSED: [[SimpleTest]]: [MySQL] 31,587 pass(es).
[ View ]
#34 drupal_db_transaction.patch8.47 KBfago
PASSED: [[SimpleTest]]: [MySQL] 31,590 pass(es).
[ View ]
#25 1007830-25.patch6.61 KBdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 31,476 pass(es).
[ View ]
#22 1007830-22.patch6.61 KBdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 31,573 pass(es).
[ View ]
#18 db_transaction_ddl-test.patch4.82 KBdrunken monkey
FAILED: [[SimpleTest]]: [MySQL] 31,528 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#18 db_transaction_ddl-test+fix.patch6.18 KBdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 31,508 pass(es).
[ View ]
#11 1007830-11.patch1006 bytesbfroehle
PASSED: [[SimpleTest]]: [MySQL] 30,515 pass(es).
[ View ]

Comments

andypost’s picture

Should we check in_transaction state when calling DDL?

bfroehle’s picture

Well, we could catch release savepoint (and rollback savepoint) errors and ignore them in the Database abstraction layer. Or, we could write some code which would essentially notify the transactions that they've been prematurely committed due to a ddl change. Or, ...?

I think this is one for the database geniuses out there.

Damien Tournoud’s picture

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.

Damien Tournoud’s picture

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.

bfroehle’s picture

Damien, this would imply adding transactions to _save() functions is not generically feasible. See #776222: Add transactions to more _save() functions.

fago’s picture

Subscribing.

drunken monkey’s picture

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.

fago’s picture

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.

fago’s picture

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?

Crell’s picture

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.

bfroehle’s picture

Status:Active» Needs review
StatusFileSize
new1006 bytes
PASSED: [[SimpleTest]]: [MySQL] 30,515 pass(es).
[ View ]

So what I guess we should do is catch PDOExceptions when we RELEASE SAVEPOINT blah and ignore error code 42000? The attached patch adds the error code to DatabaseTransaction::__destruct(), but could probably go just as well in DatabaseConnection::popTransaction(). I'm not sure which would be preferable.

See also post #43 of #776222: Add transactions to more _save() functions where I combine this patch with the lastest patch in that issue to see if it resolves the problems we were having.

bfroehle’s picture

Status:Needs review» Needs work

Marking to needs work since #43 of #776222: Add transactions to more _save() functions failed.

drunken monkey’s picture

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!

Crell’s picture

This will need unit tests, too.

bfroehle’s picture

Yes, drunken monkey is correct here. However, the problem is bigger than just catching the error is popTransaction --- as we can get errors in pushTransaction as well (I think!).

Damien Tournoud’s picture

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.

fago’s picture

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

drunken monkey’s picture

Status:Needs work» Needs review
StatusFileSize
new6.18 KB
PASSED: [[SimpleTest]]: [MySQL] 31,508 pass(es).
[ View ]
new4.82 KB
FAILED: [[SimpleTest]]: [MySQL] 31,528 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

This will need unit tests, too.

Right.

bfroehle’s picture

The tests in #18 are a great start, but we'll need to do some more to prevent the hypothetical errors like Damien suggests in #16.

Damien Tournoud’s picture

Status:Needs review» Needs work

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

drunken monkey’s picture

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.

drunken monkey’s picture

StatusFileSize
new6.61 KB
PASSED: [[SimpleTest]]: [MySQL] 31,573 pass(es).
[ View ]

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

drunken monkey’s picture

Status:Needs work» Needs review
Crell’s picture

Status:Needs review» Needs work
+++ 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.

drunken monkey’s picture

Status:Needs work» Needs review
StatusFileSize
new6.61 KB
PASSED: [[SimpleTest]]: [MySQL] 31,476 pass(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1007830-25.patch, failed testing.

bfroehle’s picture

Status:Needs work» Needs review
+++ 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 topmost transaction.
+          $this->transactionLayers = array('drupal_transaction');

Do we need to actually start the transaction again here too? That is, a call to beginTransaction().

Powered by Dreditor.

drunken monkey’s picture

#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.

drunken monkey’s picture

#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?

bfroehle’s picture

drunken monkey: It was my understanding, although I'm no expert, that when a DDL change occurs MySQL commits the transaction leaving the database thinking that there is no transaction at all. Then we go try to release a savepoint, it throws an error. To recover, we need to both start a transaction (beginTransaction()) and record in our transaction layer that we've started a transaction.

I think the patch in 25 leaves the system in a place where the transaction wrapper thinks we're in a transaction, but really we are not.

drunken monkey’s picture

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 …

MashMeisterD’s picture

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.

fago’s picture

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.

fago’s picture

StatusFileSize
new8.47 KB
PASSED: [[SimpleTest]]: [MySQL] 31,590 pass(es).
[ View ]

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.

jsenich’s picture

Subscribing.

Damien Tournoud’s picture

Status:Needs review» Needs work
<?php
+  protected function commitTransaction() {
+    return
parent::commit();
+  }
?>

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

<?php
PDO
::commit();
?>

... instead of adding a new method.

fago’s picture

Status:Needs work» Needs review

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

bfroehle’s picture

Status:Needs review» Needs work

No, it shouldn't be. See http://bugs.php.net/bug.php?id=42016

fago’s picture

StatusFileSize
new7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 31,587 pass(es).
[ View ]

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

fago’s picture

Status:Needs work» Needs review
Oceanman’s picture

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?

EndEd’s picture

subscribing

morningtime’s picture

I found this thread via Google, getting similar errors in file_usage_add() in file.inc:

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 665 of C:\Inetpub\wwwroot\123.dev\includes\file.inc).

This happens when using the Feeds module to import feeds with remote enclosure. Not exactly sure wat what point this is triggered, it only happens occasionally, e.g. for 1% of the feed items.

drunken monkey’s picture

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.

zkday’s picture

subscribing

DamienMcKenna’s picture

Seeing this via profile2.module :-\

joachim’s picture

Priority:Normal» Major
StatusFileSize
new6.71 KB
PASSED: [[SimpleTest]]: [MySQL] 29,764 pass(es).
[ View ]

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.

Jerome F’s picture

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

Damien Tournoud’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

Would like to see Crell sign of on this.

Crell’s picture

*signs his name on the dotted line*

catch’s picture

Issue tags:+needs backport to D7

tagging for backport.

joachim’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

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

catch’s picture

Version:7.x-dev» 8.x-dev
Status:Patch (to be ported)» Reviewed & tested by the community

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.

fago’s picture

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.

fago’s picture

ronan.orb’s picture

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

mrsinguyen’s picture

+1 for 7.1

webchick’s picture

Issue tags:+webchick approved

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!

Crell’s picture

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. :-)

catch’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new6.95 KB
PASSED: [[SimpleTest]]: [MySQL] 29,904 pass(es).
[ View ]

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?

webchick’s picture

StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 29,879 pass(es).
[ View ]

Oops. Forgot to include the first part. ;)

Crell’s picture

Status:Needs review» Reviewed & tested by the community

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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’s picture

Since this is a MySQL only fix, one thing we could do is parse the PDOException message to see what the MySQL error code is, and verify that it is Error: 1305 SQLSTATE: 42000 (ER_SP_DOES_NOT_EXIST).

Crell’s picture

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?

bfroehle’s picture

Title:Nested transactions throw exceptions on ddl changes» (Fixed, but minor follow-up) Nested transactions throw exceptions on ddl changes
Status:Fixed» Needs review
Issue tags:-webchick approved
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 29,905 pass(es).
[ View ]

I've implemented my idea in #66 --- namely to check that the driver specific error code is 1305 ("SAVEPOINT %s does not exist").

Crell: I'm not sure why you think the error should be 1402.

Crell’s picture

Status:Needs review» Needs 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.

bfroehle’s picture

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 29,913 pass(es).
[ View ]

I've added the word "MySQL" in the patch, so that the comment reads:

To avoid exceptions when no actual error has occurred, we silently succeed for PDOExceptions with SQLSTATE 42000 ("Syntax error or access rule violation") and MySQL error code 1305 ("SAVEPOINT does not exist").

Does that look okay?

webchick’s picture

Priority:Major» Normal
Crell’s picture

Status:Needs review» Reviewed & tested by the community

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

chx’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

paulgemini’s picture

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.

bfroehle’s picture

Title:(Fixed, but minor follow-up) Nested transactions throw exceptions on ddl changes» (Fixed, but needs yet another follow-up) Nested transactions throw exceptions on ddl changes
Status:Closed (fixed)» Active

paulgemini: What version of MySQL are you using?

The patch that was committed checks for SQLSTATE 42000 ("Syntax error or access rule violation") and MySQL error code 1305 ("SAVEPOINT does not exist"). In #76, paulgemini is experiencing SQLSTATE HY000 and MySQL error code 1305.

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

According to http://dev.mysql.com/doc/refman/5.0/en/error-messages-server.html, The value 'HY000' (general error) is used for unmapped errors.

drunken monkey’s picture

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".

Damien Tournoud’s picture

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’s picture

Status:Active» Needs review
StatusFileSize
new1.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1007830-follow-up-Remove-unnecessary-SQLSTATE-.patch.
[ View ]

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".

paulgemini’s picture

@bfroehle
Using mysql 5.1.54

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

drunken monkey’s picture

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.

paulgemini’s picture

Status:Needs review» Reviewed & tested by the community

Yep, seems to work.

Crell’s picture

Component:database system» mysql database
davidd07’s picture

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.

catch’s picture

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

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

catch’s picture

Priority:Normal» Major
Status:Fixed» Active

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.

Damien Tournoud’s picture

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.

catch’s picture

Title:(Fixed, but needs yet another follow-up) Nested transactions throw exceptions on ddl changes» Nested transactions throw exceptions when they got out of scope
beeleg’s picture

Status:Active» Needs review
bfroehle’s picture

Status:Needs review» Active

There is no patch here to review.

fago’s picture

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).
rfay’s picture

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

rfay’s picture

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!

fago’s picture

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 :(

david.mccandless’s picture

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?

starsinmypockets’s picture

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.

Damien Tournoud’s picture

Status:Active» Needs review
StatusFileSize
new3.95 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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.

starsinmypockets’s picture

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

catch’s picture

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.

starsinmypockets’s picture

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..

fago’s picture

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 :(

catch’s picture

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.

davidwhthomas’s picture

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

catch’s picture

Damien Tournoud’s picture

Status:Needs review» Needs work

Analyzing the other issue, this still fails:

<?php
$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);
}
?>
Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new11.02 KB
PASSED: [[SimpleTest]]: [MySQL] 33,772 pass(es).
[ View ]

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.

Damien Tournoud’s picture

Version:8.x-dev» 7.x-dev
StatusFileSize
new10.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1007830-rollback-commit-non-existing-transactions7.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Patch for D7.

Damien Tournoud’s picture

Version:7.x-dev» 8.x-dev

Status:Needs review» Needs work

The last submitted patch, 1007830-rollback-commit-non-existing-transactions7.patch, failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
Amarjit’s picture

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.

morningtime’s picture

I had this error:

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 664 of /httpdocs/includes/file.inc).

Then I applied the patch from #109 on Drupal 7, but the error did not go away.

Is there any way I can help find the solution? My problem occurs while inserting many files very quickly, using file_usage_add().

I'm using the Feeds module to import images. If I run two imports simultaneously, the SAVEPOINT error occurs most often.

morningtime’s picture

After some more testing, the error in #114 occurs only when I have 2 browser tabs/windows open doing an import with many file_usage_add() calls. This indicates that the database functions are getting in each other's way? No data seems lost.

basic’s picture

Version:8.x-dev» 7.x-dev
StatusFileSize
new10.71 KB
PASSED: [[SimpleTest]]: [MySQL] 37,359 pass(es).
[ View ]

Try this patch again (D7)

roborn’s picture

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

basic’s picture

Great to hear. Can we consider this RTBC?

catch’s picture

Status:Needs review» Reviewed & tested by the community

Yep, straight re-roll, RTBC.

catch’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review

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.

basic’s picture

StatusFileSize
new10.79 KB
PASSED: [[SimpleTest]]: [MySQL] 34,394 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, rollback_commit_non_existing_transactions-1007830-121.patch, failed testing.

basic’s picture

Status:Needs work» Needs review
basic’s picture

valante’s picture

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

catch’s picture

Status:Needs review» Reviewed & tested by the community

Will commit this soon if no objections.

catch’s picture

Version:8.x-dev» 7.x-dev

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

basic’s picture

#116 for D7 is ready to roll

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

bfr’s picture

Status:Closed (fixed)» Active

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.

Dave Reid’s picture

Status:Active» Closed (fixed)

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

Gastonia’s picture

Status:Closed (fixed)» Active

@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.

tim.plunkett’s picture

Status:Active» Closed (fixed)

Yes, please open a new issue.

Gastonia’s picture

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

http://drupal.org/node/1803886