Add transactions to more _save() functions

Didn't intend to change issue tags.

Patch is coming

Here's a first attempt at a patch.

Notes:
1) If a query fails in file_save, and we do a rollback, we just return an unchanged file objact. Should we return something else, like a FALSE?
2) In _menu_router_save() we return the $menu even though we don't alter it in any way. Also, it's not documented. Should we drop the return?
3) filter_format_save() uses $return for the same purpose as the other functions use $status. Also, the return is not documented.
3) path_save() has a $status, but doesn't return it, unlike other _save functions. Having it return $status, and document it in the docblock should be good, but I'm not sure if it's in the scope of this patch.

Let me know what the next steps should be, and how much cleanup should be done in the scope of this issue.

Agh. This should be better.

+++ includes/file.inc	23 Aug 2010 09:44:59 -0000
@@ -523,20 +523,29 @@ function file_load($fid) {
+  ¶

+++ includes/menu.inc	23 Aug 2010 09:45:00 -0000
@@ -2685,185 +2685,195 @@ function _menu_delete_item($item, $force
+  ¶
...
+  ¶
...
+  ¶

(and elsewhere) Trailing white-space.

Powered by Dreditor.

Is this still wanted/needed/on the table?

If yes, I can reroll properly, this was done when I was a much bigger core newb :)

#8: 776222.patch queued for re-testing.

Following up from #651240: Allow modules to react to changes to an entity.

At least all entity_save() functions should make use of transactions. (Of course others too).

Will do a reroll this afternoon.

subscribing

I need a patch for my time management skills.

Here's a reroll.

Had extra whitespace in one line.

puh, this one is really hard to review as it moves quite some code. It looks good to me though, except for:

+  catch (Exception $e) {
+    $transaction->rollback('taxonomy term');
+    watchdog_exception('taxonomy term', $e);
+    throw $e;
+  }

rollback() doesn't take any arguments (any more).

Didn't know that.
Changed.

#19: 776222.patch queued for re-testing.

#19: 776222.patch queued for re-testing.

subscribing

#19: 776222.patch queued for re-testing.

Rerolled.

#27: 776222.patch queued for re-testing.

#27: 776222.patch queued for re-testing.

Well, I'm confused.

Well the error in #27 is accurate. With a fresh copy of HEAD and #27 applied, I get the following when trying to install from scratch:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost:8888/drupal/install.php?profile=standard&locale=en&id=1&op=do StatusText: OK ResponseText: Home | Drupal @import url("http://localhost:8888/drupal/modules/system/system.theme.css?0"); @import url("http://localhost:8888/drupal/modules/system/system.messages.css?0"); @import url("http://localhost:8888/drupal/modules/system/system.menus.css?0"); @import url("http://localhost:8888/drupal/modules/system/system.base.css?0"); @import url("http://localhost:8888/drupal/modules/comment/comment.css?0"); @import url("http://localhost:8888/drupal/modules/field/theme/field.css?0"); @import url("http://localhost:8888/drupal/modules/node/node.css?0"); @import url("http://localhost:8888/drupal/modules/search/search.css?0"); @import url("http://localhost:8888/drupal/modules/user/user.css?0"); @import url("http://localhost:8888/drupal/modules/system/system.admin.css?0"); @import url("http://localhost:8888/drupal/modules/system/system.maintenance.css?0"); @import url("http://localhost:8888/drupal/themes/seven/reset.css?0"); @import url("http://localhost:8888/drupal/themes/seven/style.css?0"); Home Installation tasksChoose profile(done)Choose language(done)Verify requirements(done)Set up database(done)Install profile(active)Configure siteFinished SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist

This happened after all (or almost all) of the modules in the standard profile had been installed and configured.

Installing the minimal profile works fine.

Analysis:
I split out each file that was changed into it's own git commit and ran git bisect, which pointed to the changes in node.module as the problem.

It's strange because it's a reroll of a patch that worked fine a few weeks ago, and it just wraps some queries in transactions....

Consider the following sequence of SQL commands:

mysql> SAVEPOINT savepoint_1;
Query OK, 0 rows affected (1.35 sec)

mysql> CREATE TABLE t (c CHAR(20));
Query OK, 0 rows affected (0.09 sec)

mysql> RELEASE SAVEPOINT savepoint_1;
ERROR 1305 (42000): SAVEPOINT savepoint_1 does not exist

This means that we effectively cannot create tables during database transactions.

node.module triggers this code sequence. node_type_save() invokes comment_node_type_insert() which creates field_data_comment_body and field_revision_comment_body tables in MySQL. The table creation process commits the transaction and so causes the error when we later try to release the savepoint.

See #995820: Exception sometimes when saving and #1004332: db_set_active() commits transactions for related problems.

I'm not sure we can use db transactions for any of these _save() hooks which invoke other modules (which may then do anything, like create tables).

Or maybe we can catch DatabaseTransactionNoActiveException (or whatever exception is thrown) and choose to ignore it? (Edit: No, not feasible, as the exception is thrown midway through code execution, of course).

Maybe we can commands which commit transactions (like CREATE), nullify any pending transactions (how?) and reset the transaction depth to 0?

Edit: Corrected the above text to replace node_save() with node_type_save(), as @catch pointed out.

Seems like node creation workflow broken somehow because node_save() should NOT create any tables!

Also we need document this for contrib!!!

Great job investigating this, bfroehle!
In my opinion, telling contrib authors "don't create tables from these hooks" is perfectly ok. I mean, who even does that? It's usually a task done during module install.
I'm wondering what caused the install to break.. Maybe a git blame would find the issue that modified the node code?

Unfortunately, I don't have any more time for this issue.

is there any description of node_type_save() FSM? we have different states of type creation, suppose each DML could have own transaction but DDL has just a state - same way we could control of final state of operation

@catch in #39: Yes, node_type_save(). Comment edited to reflect this.

I created another issue, #1007830: Nested transactions throw exceptions when they got out of scope, to work out the database issues from #34 and #36.

A proof of concept for #1007830: Nested transactions throw exceptions when they got out of scope... The patch in #27 of this issue plus the patch in #11 of that issue. This should help verify that the patch in that issue is reasonable and useful.

#27: 776222.patch queued for re-testing.

Moving to d8.

And needs work as patch likely needs a reroll.

http://api.drupal.org/api/search/8/DatabaseStorageController::save has this now, I think that takes care of the entity types.
Downgrading.

Actually, I think this is fully done in D8 by now (whereas some more granular bits are still being discussed).

Given the amount of changes that were required, I don't think this can be backported.