Problem/Motivation

Better explain/document how to use database transactions in D8:

  1. That transactions can be nested and how that works on D8.
  2. That a transaction will be committed if the transaction variable gets out of scope.
  3. That transactions are only supported on database engines that support transactions and not on other engines (like MyISAM on MySQL).

Steps to reproduce

Proposed resolution

Patch
Review
Commit

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#39 788554-39.patch3.15 KBakram khan
#34 788554-34.patch3.09 KBsuresh prabhu parkala
#24 788554-24.patch2.91 KBdaffie
#18 788554-transaction-docs.patch2.96 KBsnehi
#6 788554-transaction-docs.patch2.96 KBCrell

Comments

Crell’s picture

Crell commented

The tests fail because the rollback doesn't happen. There is no extra handling for that; Drupal just doesn't die if everything works. If there's a transaction fail on ISAM we do nothing to handle that more gracefully than MyISAM does.

If the documentation implies that anywhere, that's a documentation bug.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Thanks for the clarification, Crell.

So when we update users from D6, which knows it doesn't do transactions, to D7, which assumes it can, keeping the MyISAM engine type, won't we have some pretty obscure failures?

The documentation (or at least some of it) in question is in http://api.drupal.org/api/group/database/7. I *think* since this says "transparent fallback" it implies far more than "ignores begin transaction, commit, and rollback":

Drupal also supports transactions, including a transparent fallback for databases that do not support transactions. To start a new transaction, simply call $txn = db_transaction(); in your own code. The transaction will remain open for as long as the variable $txn remains in scope. When $txn is destroyed, the transaction will be committed. If your transaction is nested inside of another then Drupal will track each transaction and only commit the outer-most transaction when the last transaction object goes out out of scope, that is, all relevant queries completed successfully.

Crell’s picture

Crell commented
Title: Transaction-related tests fail running under MyISAM engine » Improve transaction documentation

Hm. OK, I can see where it could be read that way. Also that's not entirely true anymore since we now have real transaction nesting. Sounds like we need a doc cleanup.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

A mechanism to ignore certain test cases would be nice too, so that we don't run these tests on databases that don't support transactions in the first place. We could even do that manually for now in those cases....

Crell’s picture

Crell commented

We had that before, actually, but took it out. The problem is that we were relying on the system to tell us whether it thought we were on a transaction-safe DB, which it get wrong. It's probably not worth adding that check unless we had a more reliable way to detect it.

Crell’s picture

Crell commented
Status: Active » Needs review
StatusFileSize
new 788554-transaction-docs.patch2.96 KB

How's about this?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ includes/database/database.inc
@@ -113,20 +113,32 @@
+ * case, the rollback command simply does nothing.  Note that may leave the ¶
+ * system in an inconsistent state as the transaction is not rolled back.
+ * Unfortunately there is no way around that on MyISAM tables. That is one of ¶

Trailing whitespaces :)

+++ includes/database/database.inc
@@ -144,11 +156,11 @@
  *     // Something went wrong somewhere, so flag the entire transaction to
  *     // roll back instead of getting committed.  It doesn't actually roll back
  *     // yet, just gets flagged to do so.
- *     $txn->rollback();
+ *     $transaction->rollback('my_checkpoint');
  *   }
  *
- *   // $txn goes out of scope here.  If there was a problem, it rolls back
- *   // automatically.  If not, it commits automatically.
+ *   // $transaction goes out of scope here.  If it was not already rolled back
+ *   // it will be committed.

The first paragraph is still wrong. I already tried to improve this in #711108: Richer exception reporting for watchdog(), so it will need to be updated when either this or the other patch gets commited.

Other than that, this looks good to me.

78 critical left. Go review some!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Crell’s picture

Crell commented
Assigned: Unassigned » Crell

Bad me for letting this fall off my radar. Hopefully I'll remember if I assign it to me.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

Ping? This is an important issue. Thanks, Crell.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

One more documentation issue pertaining to transaction #1221772: Transaction database settings is misleading in settings.php

Tor Arne Thune’s picture

Tor Arne Thune commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Documentation, +Needs backport to D7
kattekrab’s picture

kattekrab commented

Hrmmm... time to move this to D9?

Crell’s picture

Crell commented

No, this is all doc block cleanup, it's still valid for D8. I'm just a loser when it comes to catching up on docs. :-(

stewartsmith’s picture

stewartsmith commented

My advice would be:
1) drop MyISAM support.
InnoDB has been available and stable for well over a decade and finding a MyISAM only MySQL instance is basically impossible. My prediction is that in one or two more major releases, MyISAM will disappear almost completely from MySQL. Note that just about all database as a service offerings from cloud providers are InnoDB only and you loose support if you use MyISAM.
i.e. error out if MyISAM - and if MyISAM, in upgrade, upgrade to InnoDB. Who knows what kind of bugs/exploits you open users up to with untested code paths with invalid/inconsistent data in the database caused by lack of rollback.
2) be careful with what you name checkpoints, two checkpoints of the same name means rollback may/may not do what you intend it to.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Assigned: Crell » Unassigned
Issue summary: View changes
snehi’s picture

snehi commented
Assigned: Unassigned » snehi
snehi’s picture

snehi commented
Assigned: snehi » Unassigned
StatusFileSize
new 788554-transaction-docs.patch2.96 KB

Meanwhile removing trailing spaces.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review

Want to engage the bot here. Thanks for re-rolling the patch.

Status: Needs review » Needs work

The last submitted patch, 18: 788554-transaction-docs.patch, failed testing.

The last submitted patch, 18: 788554-transaction-docs.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 18: 788554-transaction-docs.patch, failed testing.

daffie’s picture

daffie commented
Status: Needs work » Needs review
StatusFileSize
new 788554-24.patch2.91 KB

Rerolled the patch. The file includes/database/database.inc to core/lib/Drupal/Core/Database/database.api.php.

Also removed some double spaces between sentences.

I also reviewed the patch and for me it is RTBC.

daffie’s picture

daffie commented
Title: Improve transaction documentation » Improve documentation for how to use database transactions in database.api.php
Issue summary: View changes

Updated issue title and summary to better explain what this issue is about.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Version: 8.9.x-dev » 9.3.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll, +Bug Smash Initiative

The patch also refers to db_transaction which has been removed.

suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 788554-34.patch3.09 KB

A re-rolled patch against 9.3.x. Please review.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs reroll

@Suresh Prabhu Parkala To help reviewers add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff in the handbook. If you think a diff is not needed, just add a comment stating why.

This needs work for #33, that db_transaction no longer exists in core.

Also, why does the patch have code changes as well?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 788554-39.patch3.15 KB

Patch against 10.1.x address #7 as well needs review

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Per #35

This needs work for #33, that db_transaction no longer exists in core.

Also, why does the patch have code changes as well?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Category: Bug report » Task
Related issues: +#3364706: Refactor transactions

This is a task, the current documentation isn't wrong, just not as comprehensive as it could be.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.