Make transactions more flexible and useful [#1185780]

Problem/Motivation

(Original report by @Damien Tournoud)

Our transaction support relies on the synchronous destructors of PHP and as a consequence relies on the stack of function calls. The typical call is:

function a() {
  $transaction = db_transaction();
  b();
  // Transaction will commit as soon as we exit the function because $transaction gets out of scope.
}

function b() {
  // Open a nested transaction.
  $transaction = db_transaction();
  ...
  // Transaction will be popped as soon as we exit the function because $transaction gets out of scope.
}

The problem is that it only works with some limited use cases.

If you want to implement row-level locking, for example, you need to start a transaction, load a row in FOR UPDATE, save the row and close the transaction. But in the typical page execution model of Drupal, entities tends to be loaded early (generally by the menu system), and are saved elsewhere during the execution of the page (generally in a submit function). Those two operations are not in the same callstack at all, so you have no opportunity to start a transaction before loading and commit it after saving.

Let's try to extend the transaction system to support this use case.

Proposed solution

(From #1185780-8: Make transactions more flexible and useful.)

This patch allows transactions to be committed / rolledback in a different order that they have been created. It is especially useful for situations when you want to take advantage of row-level locking. In that case you need to start a transaction in the load controller of the entity that can stay open up until save, but because those two operations are not in the same call stack (load is typically done in the menu system while save typically happens in a form submit function, you are not guaranteed to pop and push in the proper order.

This patch covers the following cases:

Committing in the proper order

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction2); // RELEASE SAVEPOINT savepoint_0
unset($transaction); // COMMIT

Committing in a different order

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // Nothing happens, just marking the transaction as committable
unset($transaction2); // RELEASE SAVEPOINT savepoint_0; COMMIT

Rolling back an inner transaction before committing the outer transaction

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
$transaction2->rollback(); // ROLLBACK TO SAVEPOINT savepoint_0;
unset($transaction); // COMMIT

Rolling back an inner transaction after committing the outer transaction

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // Nothing happens, just marking the transaction as committable
$transaction2->rollback(); // ROLLBACK TO SAVEPOINT savepoint_0; COMMIT

Rolling back the outer transaction while an inner transaction is active

In that case, we throw an exception after rolling back, because there is no way we can guarantee that later queries will respect the transaction contract:

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // ROLLBACK; but Exception: rolled back a parent transaction when the child transaction was active
$transaction2->rollback(); // Exception: transaction not active

Remaining tasks

Committer approval.

User interface changes

None.

API changes

All the changes in that patch are backward compatible. Transactions pushed and popped in the proper order behave exactly the same way as before. Transactions pushed and popped in a different order behave the way one would expect them to.

Comment	File	Size	Author
#28	1185780-transaction-order-d7.patch	12.21 KB	Damien Tournoud
#28
#27	1185780-transaction-order.patch	12.21 KB	Damien Tournoud
#27
#21	1185780-transaction-order.patch	12.29 KB	Damien Tournoud
#21
#9	1185780-transaction-order.patch	12.31 KB	Damien Tournoud
#9
#5	1185780-transaction-order.patch	12.31 KB	Damien Tournoud
#5
#2	1185780-transaction-order.patch	11.4 KB	Damien Tournoud
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 June 2011 at 01:08

So, the problem here is that depending on the code path, transactions can be pop-ed in a different order they are push-ed. Drupal can have arbitrary complex code execution paths and we have no way of controlling that.

Currently, if you have two transactions started and the first one is destructed, the whole transaction stack will be destroyed:

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // COMMIT
unset($transaction2); // Silently fails

I suggest we make the database layer wait until the second transaction gets either committed or rolled back:

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // Nothing happens, we just store the information.
unset($transaction2); // RELEASE SAVEPOINT savepoint_0; COMMIT

Comment #2

Damien Tournoud CreditAttribution: Damien Tournoud commented 26 June 2011 at 11:23

Status:	Active	» Needs review
Issue tags:		+Needs backport to D7

File	Size
1185780-transaction-order.patch	11.4 KB

First patch for review.

Comment #3

26 June 2011 at 11:55

Status:

Needs review

» Needs work

The last submitted patch, 1185780-transaction-order.patch, failed testing.

Comment #4

Crell CreditAttribution: Crell commented 26 June 2011 at 22:38

Subscribing.

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 10:02

Status:

Needs work

» Needs review

File	Size
1185780-transaction-order.patch	12.31 KB

Fixing MySQL that was stuffing array('drupal_transaction') instead of array('drupal_transaction' => 'drupal_transaction') inside transactionLayers.

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 10:17

Title:

Transactions are inflexible

» Make transactions more flexible and useful

Comment #7

catch

he/him

English

CreditAttribution: catch commented 27 June 2011 at 10:47

Subscribing.

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 10:59

Summary of the patch

This patch covers the following cases:

Committing in the proper order

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction2); // RELEASE SAVEPOINT savepoint_0
unset($transaction); // COMMIT

Committing in a different order

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // Nothing happens, just marking the transaction as committable
unset($transaction2); // RELEASE SAVEPOINT savepoint_0; COMMIT

Rolling back an inner transaction before committing the outter transaction

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
$transaction2->rollback(); // ROLLBACK TO SAVEPOINT savepoint_0;
unset($transaction); // COMMIT

Rolling back an inner transaction after committing the outter transaction

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // Nothing happens, just marking the transaction as committable
$transaction2->rollback(); // ROLLBACK TO SAVEPOINT savepoint_0; COMMIT

Rolling back the outter transaction while an inner transaction is active

In that case, we throw an exception after rolling back, because there is no way we can guarantee that later queries will respect the transaction contract:

$transaction = db_transaction(); // BEGIN TRANSACTION
$transaction2 = db_transaction(); // SAVEPOINT savepoint_0
unset($transaction); // ROLLBACK; but Exception: rolled back a parent transaction when the child transaction was active
$transaction2->rollback(); // Exception: transaction not active

API changes

All the changes in that patch are backward compatible. Transactions pushed and poped in the proper order behave exactly the same way as before. Transactions pushed and poped in a different order behave the way one would expect them to.

Comment #9

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 11:02

File	Size
1185780-transaction-order.patch	12.31 KB

Commitable => Committable (from catch)

Comment #10

David Strauss

he/him

CreditAttribution: David Strauss commented 27 June 2011 at 17:40

I'm not sure I agree with this approach. If transactions aren't properly nested, that's a design problem in the underlying code. In the case of the load/modify/save flow, the function controlling the overall load/modify/save should start its own transaction before calling load(). The load() operation could still have an option to SELECT ... FOR UPDATE, which would only get invoked if the caller requested it. The save() operation might start its own transaction/savepoint, but that would be nested within the load/modify/save transaction.

Comment #11

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 17:58

In the case of the load/modify/save flow, the function controlling the overall load/modify/save should start its own transaction before calling load().

@David: How do you see that working? The entity is generally loaded first by the menu system, and goes through a *ton* of different processes (rules, hook_entity_load(), view, form generation, etc.) each of them can decide that it finally wants to modify and save it. It's a pipe dream to think that we could possibly be in a linear process.

When studying how to lock entities for Drupal Commerce, we quickly came to the conclusion that we need unconditional pessimistic locking (a la Active Records pessimistic locking) precisely for that reason: (1) there is no way for us to know on load that any part of Drupal might want to save this loaded entity down the road, (2) we cannot do optimistic locking has there is no way for us to restart the whole processing pipeline if save fails (how do you unsend an email?).

Comment #12

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 18:00

So while I agree that in an ideal world we should have a cleanly separated load / modify / save workflow, this doesn't match how Drupal works at all. Our second best is to allow transactions to be poped in a different order that they have been pushed, which is the point of this patch.

Comment #13

David Strauss

he/him

CreditAttribution: David Strauss commented 27 June 2011 at 21:21

As distasteful as I find this use of transactions, we should either implement Damien's patch or make reversing the order throw an exception. The current behavior is just dumb because it's neither useful nor failing loudly for developers. At least this patch makes it do something useful. So, I guess that's a +1. We can consider removing the capability and failing loudly in the future if we can handle the use cases Damien raises more elegantly.

Regarding the Drupal Commerce case in particular, this sort of flow would be ideal *within the save() function* and wouldn't require out-of-order transactions:

(1) Start transaction
(2) SELECT ... FOR UPDATE on the critical row(s)
(3) Allow modification of the entity-to-be-saved. (optional)
(4) Allow a "veto" on saving the entity that stops the process and rolls back. (optional)
(5) Call any hooks dependent on the ability to successfully save the entity, like sending emails.
(6) Actually save the entity.
(7) Commit

The key is integrating irreversible side-effects deeper in the call stack of the save() logic. This design also avoids problems associated with loading entities using FOR UPDATE when they may or may not be updated. Gratuitous use of FOR UPDATE has nasty effects on concurrency at higher transaction isolation levels (though not at the default isolation level on MySQL+InnoDB) and can also promote catastrophic levels of lock escalation (though not on MySQL+InnoDB or PostgreSQL). SQLite "escalates" the FOR UPDATE lock to a lock on the entire database, which is pretty much the only way SQLite can lock, anyway.

And, at least on MySQL [1] and PostgreSQL [2], FOR UPDATE has all kinds of fun interactions with save points. Specifically, MySQL always holds locks until the end of the overall transaction, regardless of save point rollbacks. PostgreSQL tends to "forget" locks established before the savepoint that code has rolled back to. This may cause hazards in the Drupal Commerce code when you don't control the code path between the FOR UPDATE lock and the actual saving of changes.

Is Drupal Commerce truly dependent on requiring every load() to guarantee the subsequent ability to save()?

[1] http://dev.mysql.com/doc/refman/5.5/en/savepoint.html
[2] http://www.postgresql.org/docs/9.0/interactive/sql-select.html#SQL-FOR-U...

Comment #14

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 June 2011 at 22:04

Regarding the Drupal Commerce case in particular, this sort of flow would be ideal *within the save() function* and wouldn't require out-of-order transactions: [...]

The save operation by itself is actually not too much a concern. The real issue is in all the operations that occur between the load and the save, that can take a significant amount of time.

Typical use case: a set of rules is executed when the users complete the checkout process that result in the status of the order to change (the order object is loaded before the execution of the rules, and saved at the end); at the same time, a payment gateway (for example Paypal IPN) pings back the site to confirm full payment, another set of rules is executed that can result in the status of the order to be changed to a different status.

In this use case the end-result is undefined. And we cannot just do an optimistic locking (as in Active Record) when saving because there is no way we can rollback and reprocess everything (how do you un-send an email, for example?).

Comment #15

chx CreditAttribution: chx commented 28 June 2011 at 23:06

Status:

Needs review

» Reviewed & tested by the community

With David's reluctant +1 here's a relucatant RTBC: both him and me wish we do not need to do this. But NOT doing it is worse and doing "something" else equals tearing apart Drupal into very small pieces and putting it together again with a way more straightforward code path and MUCH less interdepencies.

Comment #16

catch

he/him

English

CreditAttribution: catch commented 29 June 2011 at 00:00

Status:

Reviewed & tested by the community

» Needs work

Sorry I still haven't properly reviewed but a non-proper review from phone spotted

poping - popping

outter - outer

Comment #17

Crell CreditAttribution: Crell commented 29 June 2011 at 00:57

Question. How exactly will we decide if we need to add FOR UPDATE to the loading query? Especially when there could be a dozen related records across many tables, added by different modules?

I don't see how we could add FOR UPDATE in the first place, which seems like a prerequisite for this patch to be relevant.

Comment #18

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 June 2011 at 01:12

@Crell: I'm going to add FOR UPDATE in Drupal Commerce entity controller. I'm not talking about adding that to core at all.

Comment #19

Crell CreditAttribution: Crell commented 29 June 2011 at 20:41

So all Commerce entities would by default do a FOR UPDATE load, thus locking them for... the duration of the request?

Comment #20

Damien Tournoud CreditAttribution: Damien Tournoud commented 29 June 2011 at 21:00

> So all Commerce entities would by default do a FOR UPDATE load, thus locking them for... the duration of the request?

Most likely only orders, but yes.

Comment #21

Damien Tournoud CreditAttribution: Damien Tournoud commented 30 June 2011 at 09:17

Status:

Needs work

» Needs review

File	Size
1185780-transaction-order.patch	12.29 KB

Fixed spelling of outer and popping.

Comment #22

catch

he/him

English

CreditAttribution: catch commented 2 July 2011 at 05:15

Status:

Needs review

» Reviewed & tested by the community

Restoring rtbc status. I don't have a strong opinion on the patch and as David says teansaction isolation settings can affect for update behaviour pretty dramatically, but we're not adding anything luke that to core here, just facilitating it, and the tests look comprehensive.

Comment #23

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 5 July 2011 at 05:15

Priority:

Normal

» Major

I feel pretty out of my depth on this one, but have pinged Dries to take a look.

I'm also raising priority to "major" so that this doesn't get lost.

Comment #24

Crell CreditAttribution: Crell commented 6 July 2011 at 00:01

I'm OK with this patch in principle at this point, and largely defer to David on the validity of the approach. However, is DatabaseTransactionRollbackOtherActiveTransactionsException really the best we can do? :-) I'm all for descriptive exception names but yeesh.

Comment #25

Damien Tournoud CreditAttribution: Damien Tournoud commented 8 July 2011 at 13:41

@Crell any other suggestion?

Comment #26

Crell CreditAttribution: Crell commented 8 July 2011 at 23:19

DatabaseTransactionOutOfOrderException?

Comment #27

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2011 at 09:16

File	Size
1185780-transaction-order.patch	12.21 KB

Rerolled with the new exception name.

Comment #28

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2011 at 09:19

File	Size
1185780-transaction-order-d7.patch	12.21 KB

Drupal 7 version (it cherry picks just fine).

Comment #29

catch

he/him

English

CreditAttribution: catch commented 14 July 2011 at 03:45

This is likely blocking #687180: Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content, although it doesn't actually fix the issue I ran into there yet.

Comment #30

Damien Tournoud CreditAttribution: Damien Tournoud commented 14 July 2011 at 08:17

@catch: that will not fix this bug, but allow us to fix it, see #1007830-88: Nested transactions throw exceptions when they got out of scope.

Comment #31

catch

he/him

English

CreditAttribution: catch commented 14 July 2011 at 08:29

@Damien, yep, hence blocking rather than fixing :)

Comment #32

Crell CreditAttribution: Crell commented 14 July 2011 at 16:00

I am good with #27.

Comment #32.0

xjm

she/her

English

CreditAttribution: xjm commented 24 July 2011 at 10:55

Issue summary:

View changes

Added issue summary from comment #8.

Comment #32.1

xjm

she/her

English

CreditAttribution: xjm commented 24 July 2011 at 10:57

Issue summary:

View changes

Fixed the same spelling errors that were fixed in the later roll of the patch.

Comment #33

xjm

she/her

English

CreditAttribution: xjm commented 24 July 2011 at 10:59

Added issue summary.

Comment #33.0

xjm

she/her

English

CreditAttribution: xjm commented 24 July 2011 at 10:59

Issue summary:

View changes

Made headers consistent with template.

Comment #33.1

xjm

she/her

English

CreditAttribution: xjm commented 25 July 2011 at 20:12

Issue summary:

View changes

Credited sources of the summary information. I am not that smart. ;)

Comment #33.2

xjm

she/her

English

CreditAttribution: xjm commented 25 July 2011 at 20:13

Issue summary:

View changes

typo

Comment #34

Damien Tournoud CreditAttribution: Damien Tournoud commented 26 July 2011 at 14:28

So, this has been RTBC since July 2, and a final patch (with some wording change) available and RTBC since July 12. Are we really saying that Drupal 7.5 is going to ship tomorrow without this patch?

Comment #35

catch

he/him

English

CreditAttribution: catch commented 26 July 2011 at 15:45

Since webchick deferred to Dries and it's three weeks since he last committed a patch that seems very likely.

Comment #36

Dries CreditAttribution: Dries commented 1 August 2011 at 02:19

I wasn't committing patch because I was on vacation for 10 days, and because we didn't meet our thresholds.

I just tried to apply the patch in #27 but got errors. I'll ask for a re-test.

Comment #37

Dries CreditAttribution: Dries commented 1 August 2011 at 02:20

#27: 1185780-transaction-order.patch queued for re-testing.

Comment #38

catch

he/him

English

CreditAttribution: catch commented 1 August 2011 at 05:32

@Dries, it's good to have you back, but I don't think your response answers Damien's frustration with the lack of timely feedback on this issue.

This is a major bug report, hence committing it is necessary in order to be under the thresholds. If major bugs don't get committed because we're over the major bug threshold, that's a situation that is going to be very hard to get out of.

It's also blocking work on other issues like #1007830-88: Nested transactions throw exceptions when they got out of scope, which in turn blocks #687180: Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content - both of which are major bugs.

It was RTBC on July 2nd, while webchick apparently pinged you on July 5th. There's a good clear couple of weeks since then when you weren't on holiday. This is also not the only RTBC major issue in the queue that has been deferred by webchick and also been RTBC for 4 weeks or more without a response (which at the moment means skipping a point release).

When those issues sit around for a long time in the queue without feedback, it very much lessens people's motivation to work on other core issues. When bugs are blocking contrib modules or other core patches, it has a knock-on effect regardless of individual motivation; the issues are just stalled with no indication of when it might be possible to work on them again. It's possible to do stacked patches (for core at least) but that's a pain in the arse, and also requires some faith that the blocking patch will go in without further changes, which is never guaranteed.

Since there is no Drupal 8 branch maintainer, and webchick is mostly not committing D8-only patches (and some backportable patches that have broad implications), that leaves you as the sole bottleneck for issues like this.

Comment #39

Dries CreditAttribution: Dries commented 1 August 2011 at 10:07

I agree I dropped the ball on this one. I'll work with webchick on this -- she needs to ping me harder and more persistently. In addition, I'll make sure to make enough time for this. I'll talk to webchick today to see how we can prevent this going forward. I'm not blaming her because she does a great job; this is something we can prevent going forward by making some changes on how we work together.