Each connection should have its $layers tracked separately. Otherwise, we may begin and end transactions on the wrong connection or improperly nest transactions. (Yes, this is also bug in my original contributed transaction code, which I'm fixing.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David Strauss’s picture

Status: Active » Needs review
FileSize
13.67 KB
Crell’s picture

Status: Needs review » Needs work

Let's set transactionLayers to 0 on definition rather than in the constructor.

inTransaction()'s docblock should document its return with @return; the first line should be descriptive.

Should willRollBack() throw an exception if there is no transaction active? I'm pondering if we shouldn't just return FALSE, since, well, there will be no roll back.

Why are we cancelling a rollback if we start a new transaction?

ExplicitTransactionsNotSupportedException should be better documented as to what it's useful for. Maybe needs a name, too.

Docblock for DatabaseTransactionTestCase is wrong.

While we're at it, let's expose transactions. :-) db_transaction($required = FALSE) { ... }

moshe weitzman’s picture

Nice work. And a simpletest as well.

I'm not really qualified to review this but I know I want it :)

Crell’s picture

Assigned: Unassigned » Crell
Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
19.02 KB

OK, I'm taking this issue over. :-)

Attached patch does the following compared to HEAD, as discussed with David back in Szeged.

- Move most of the transaction handling logic into the DatabaseStatement class, so that it's part of the connection object.

- Overrides the PDO connection methods deliberately so that it is impossible to start transactions without using our system. Allowing people to bypass them would open up the potential for all sorts of mess with transactions closing at unexpected times.

- Introduce a db_transactions() utility method. I included the ability to set a different target mostly for completeness should we ever add more targets than just default and slave, which I want to do time permitting.

- Adds a new unit test class for transactions. This one is a little weird... The problem is that the correct behavior of many transaction-related tests is different depending on if the underlying database supports transactions. There is a proper way for it to fail in that case. Therefore there are 5 unit tests: One for normal commit that is the same for both cases, and then two others for each case; tests that expect transactions to be supported will simply skip themselves if transactions are not enabled and those that test how Drupal behaves when transactions are not supported do the same if transactions are enabled.

While that may seem weird, it is in fact by design. Since we can't require that all databases we run on support transactions we have a very clearly defined failure mechanism: Specifically, if a transaction is "optional" (the default) then rollback will do nothing. If a transaction is required, Drupal throws an exception if it cannot be opened. These unit tests cover all of those cases, with the downside that some unit tests only run in some cases. I see no way around that.

This needs to get in before we can start sprinkling transactions through core, so I am elevating it to critical as it is blocking a lot of follow-on patches.

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

Grrr... Come on bot, tell me WHICH test failed. All of the database-related tests pass on my computer.

Anonymous’s picture

Status: Needs work » Needs review

patch applied cleanly, and i got 7461 passes, 0 fails, and 0 exceptions.

bad bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

David Strauss’s picture

I'm updating the patch tonight. I don't want to hear anything about this not being in core yet when I'm at the fields sprint.

David Strauss’s picture

Oh, just noticed that Crell took over the issue.

David Strauss’s picture

FileSize
17.8 KB

Applying Crell's updated patch failed for me. Here's a re-roll of the patch against HEAD.

Tests show: 7573 passes, 0 fails, and 0 exceptions

Crell’s picture

Status: Needs work » Needs review

Hey bot, what do you think now?

Status: Needs review » Needs work

The last submitted patch failed testing.

David Strauss’s picture

"Failed to run tests"

What is that supposed to mean?

David Strauss’s picture

Status: Needs work » Needs review

Will this make the test server try testing again?

David Strauss’s picture

The tests passed on re-test. This is now ripe for review.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dries’s picture

The code looks good, but I'm not sure I fully grok transactionInnerLayer and transactionOuterLayer. Can you extend the phpDoc a bit, or rather, make the phpDoc a bit more useful?

In general, I think, the phpDoc could be improved upon. It could do a better job giving a high-level overview of the transaction model/api, as well as some additional theory on the layer-system. We should answer the 'why?' here...

David Strauss’s picture

FileSize
18.49 KB

Here's a re-roll, but I can't install because of this error: "PDOException: SELECT COUNT(*) AS expression FROM {menu_links} menu_links WHERE (module = :db_condition_placeholder_29) AND (link_path = :db_condition_placeholder_30) - Array ( [:db_condition_placeholder_29] => system [:db_condition_placeholder_30] => ) SQLSTATE[42000]: Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause in menu_link_save() (line 2061 of /home/straussd/domains/transactions.fkbuild.com/public_html/includes/menu.inc)."

I'm assuming that's a result of the TRADITIONAL SQL mode patch.

@Dries: I extended the test documentation.

David Strauss’s picture

Status: Needs work » Needs review
David Strauss’s picture

All tests pass with my patch, as long as I revert the SQL mode changes committed earlier today.

David Strauss’s picture

Assigned: Crell » David Strauss
Dries’s picture

Status: Needs review » Needs work

--$this->transactionLayers; should be written as $this->transactionLayers-- IMO. It is nicer.

Some debug code left at + //return new $class_type($this);.

Typos: and begin a should be 'begins', I think.

The documentation is somewhat better, but still not perfect, IMO.

Please take another look at this patch. I committed the fix for the TRADITIONAL SQL mode patch.

Crell’s picture

RE --$transactionLayers, in PHP a prefix increment/decrement operator is slightly faster than a postfix increment/decrement. I'm not entirely sure why, but I've seen benchmarks that claim so. If you think that's over-optimization OK, but I generally use the prefix version if I have a choice for that reason.

David Strauss’s picture

FileSize
21.4 KB

* Crell is correct about the prefix increment and decrement operators, but I don't think the question a big deal, either way.

* I don't think the commented code was for debugging. I think it was detritus from a previous revision. Nevertheless, I've removed it.

* The text "and begin a" is correct. With our transaction abstraction, the inner transaction should not commit the outer transaction or begin a new transaction. But without the nesting logic, it would do both (in that order). I've rewritten the surrounding docs here.

* I've added general overall documentation to the test class, but this documentation might fit better into the main implementation.

Here's a re-roll, for good measure.

David Strauss’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

David Strauss’s picture

Status: Needs work » Needs review
FileSize
21.2 KB

Let's try a patch that doesn't update Bazaar settings.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
23.63 KB

The docblock for $transactionalDDLSupport needs to have an extra line between the first and second lines of the description. Various other docblocks have multi-line descriptions.

The transaction exceptions don't do anything PDO-specific, so they shouldn't extend PDOException but just Exception.

I've also moved the lengthy description of how the transactions work from the unit test to the main database docblock, as that way it will show up on the proper page on api.drupal.org. I think we can safely assume that someone reading the unit test will have ready access to that documentation as well.

Since this is just documentation cleanup, I'm going to go ahead and bump this to RTBC. All tests pass for me, so if the bot likes it then we're good to go. :-)

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to be a pain here, but the documentation _still_ needs work. There is a lot of good information on why we implemented things the way we implemented them, but we are still low on information as how to use the transaction support (e.g. how to do use transactions across multiple functions?). Please add a least one practical example to the documentation.

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Dries, extensive use documentation belongs in the handbooks, not in docblocks the size of Montana. :-) There's already extensive use documentation for the rest of the DB layer in the handbooks and transactions will be included there as well. The main database.inc docblock in this patch contains as much information about transactions as it does about general queries. In fact I think it contains more, since it includes the background on synchronization in C++ and Java, too.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I _do_ think we need a bit more practical documentation in the code. If the length of the documentation is an issue, I'd be OK to see us compress the "background information" (e.g. comparisons with C/C++ and Java) a bit.

Josh Waihi’s picture

 $txn = db_transaction();

    // Insert a single row into the testing table.
    db_insert('test')
      ->fields(array(
        'name' => 'David' . $suffix,
        'age' => '24',
      ))
     ->execute()

This at first glance looks like $txn isn't actually being used. Without tracking down what db_transaction() does, $txn looks like a defined variable that doesn't get used. Could we do something like:

function outerTxn() {
  db_transaction()
    ->insert('test')
      ->fields(array('name', 'age'))
      ->values(array(
        'name' => 'David',
        'age'  => 24
      ))
      ->values(array(
        'name' => 'Tom',
        'age'  => 25
      ))
      ->execute();
  innerTxn();
  db_transaction()->commit();
}

function innerTxn() {
  db_transaction()
    ->insert('test')
      ->fields(array(
          'name' => 'David',
          'age'  => 24
      ))
  db_transaction()->commit();
}

outerTxn();

This just looks like transactions are being used and not that some random variable has been set and not used.
another suggestion still:

try {
  db_transaction()
    ->update().....
    .........
    ........
  db_transaction()
    ->commit();
} catch (NoCommitNestedTransaction as $e) {
  // handle if was a nested transaction

} catch (TransactionFailed as $e) {
  // handle failed transaction i.e rollback

}

//call the exceptions whatever but you get the idea.

I'm sure there are some reasons why this isn't happening at the moment, I'd like to know them. Its looking really good, just think that the code in practice needs to be more self explanatory

Crell’s picture

Status: Needs work » Needs review
FileSize
22.53 KB

I've expanded the code comment to be a complete example. It's totally contrived, but until we start using transactions in core I couldn't come up with anything better and this lets us show and document exactly what we want to.

I also moved the language comparisons out of the code and to a new handbook page that is a copy of the code example, plus the longer description from the previous patch: [#355875]. We can expand further there as needed.

I discussed the syntax question with Josh in IRC and pointed him toward the new handbook page, and he's OK with this syntax now, too. :-)

Josh Waihi’s picture

Status: Needs review » Needs work

There seems to be a few things over looked such as the occasion a query fails. what happens? I know a PDO Exception is thrown, it should at least be documented how to handle that. What happens if your nested 2 layers in and a query fails, obviously all the queries in the transactions will fail but how does the outer transaction know? does it need to know? I think so. If a inner query fails, it can deal with the exception but the outer transaction will still have no clue the transaction has failed.

Well, actually, once a single query fails, all other queries will fail also. But yeah, maybe some more thought needs to go into this and at the very least added to the comments.

Crell’s picture

Status: Needs work » Needs review

And of course I remember the answer t that question after Josh goes to sleep. :-) If you roll back a transaction, it doesn't really roll back. It gets marked to roll back when the outer-most transaction layer is popped. So all is well.

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

ok I'm happy - would like an example with exception handling but am happy to mark this RTBC without

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks all!

Josh Waihi’s picture

Assigned: David Strauss » Josh Waihi
Status: Fixed » Needs review
Issue tags: +PostgreSQL Surge
FileSize
1.37 KB

just tried installing CVS HEAD with PostgreSQL and failed because of transaction stuff. turned out to be a bit of missed code and clean up

Josh Waihi’s picture

er, actually, I should use parent here rather than self, though self worked. Would have been great to have had a Postgres test bed in this situation since a MySQL testbed that isn't using transactions isn't exactly helpful.

drewish’s picture

So before applying the patch installation on pgsql fails with the following error:
Fatal error: Call to undefined method PDO::starttransaction() in /Users/amorton/Sites/dh/includes/database/database.inc on line 854 Fatal error: Call to undefined method DatabaseTransaction::commit() in /Users/amorton/Sites/dh/includes/database/query.inc on line 694

After applying the patch and installation completes but I couldn't get the tests to run. Not sure if there's another issue involved with that problem.

drewish’s picture

I should also add that the site is usable, I can create content etc, but the tests fail right away with a HTTP 500 error.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.91 KB

Also fixed an issue in testTransactionsNotSupported(): apparently the test was never run on a database that actually supports transactions... which is quite worrying.

I'm not seeing any of the behavior described by drewish. This patch is ready to go for me.

Josh Waihi’s picture

Cool, thanks guys, I didn't have the time to run the tests - good that you picked this up.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Damien Tournoud’s picture

Assigned: Josh Waihi » Unassigned
Status: Fixed » Reviewed & tested by the community
FileSize
746 bytes

Another follow-up: InsertQuery was not fixed either.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -PostgreSQL Surge

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