Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate

I think it would be possible to set the transaction isolation level for a connection. Doctrine's DBAL allows this in Connection::setTransactionIsolation(). Then it could be possible to change between isolation levels depending on what's necessary.

Here is something I banged out in a short while. I am not sure if the function on Connection should be abstract. If in RC, then it would break drivers. A default function could be provided. Also not sure if the Trait has that much value. No tests yet and this patch isn't really tested at all.

I forgot to add the new file in.

Having the ability to alter the isolation level is a must. Because it could certainly be very important to control this while installs, for example, or even on some very specific jobs (like going to serializable when manipulating monney on a real accountability module, some day).

But curently the way fields are saved with delete+insert is doing too much locks on MySQL in REPEATABLE READ mode. as referenced in #1650930. We could also say that the current way of handling locks in transation (suicide) is not very robust (one could instead try to restart failing transactions), and deadlocks are quite frequent on big Drupal websites, not only because of the fields saves. READ COMMITTED is maybe a better default for Drupal, IMHO, especially if you can elevate this level whil performing critical tasks like installations.

Okay, let's try adding a setter to the Transaction class (and move stuff from previous patch to Transaction). This will probably fail the tests and/or crash horribly.

I removed the system requirements as well.

Fix namespace issue in test.

Flip back to needs work.

Fix typos.

I guess since RC1 is imminent that means most of those abstracts need to be generic implementations so that contrib drivers that support transactions don't break post rc1.

More typos as I run via my drupalci local instance.

Also found a bug in MySQL < 5.5 where set transaction affects the entire session even if it's only in a transaction.

Hmm... It looks like MySQL and MySQL-likes have the behavior of calling SET TRANSACTION outside of a transaction ("Active sql transaction: 1568 Transaction isolation level can't be changed while a transaction is in progress"). While PostgreSQL and SQL Server allow for setting isolation level within transactions.

So I think that we need to move this back to Connection class for the drivers, and that the mysql driver shouldn't allow attempting to change transaction isolation when inside a transaction.

Looking at the docs more closely, mysql and postgresql implement the opposite of each other, and so each driver must extend the constuctor of Transaction before (or after) the transaction is begun respectively. I am not sure about sqlite, so we'll see.

This should also be backwards-compatible and now startTransaction() can be called by the application. I like that better as the setter can be a protected method.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Transaction.php
   @@ -9,4 +9,71 @@
   +    $level_string = $this->connection->query("SELECT @@tx_isolation")->fetchField();
   

   This may not be doing what I expect it to do because it's the session variable. I do not think there is a way to get the isolation level that was set for a transaction in MySQL. :(

   Possibilities:

   1. Set transaction as a session and unset it on destruct. Might be OK, but not ideal.
   2. Get rid of the getter and just test scenarios similar to https://github.com/ept/hermitage.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Transaction.php
   @@ -9,4 +9,71 @@
   +        throw new \InvalidArgumentException('Invalid transaction isolation level provided.');
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Transaction.php
   @@ -9,4 +9,68 @@
   +        throw new \InvalidArgumentException('Invalid transaction isolation level provided.');
   

   I think that this should be an assert() instead of an Exception these days. Apologies to AkiTendo.

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/TransactionIsolationTest.php
   @@ -0,0 +1,74 @@
   +      $this->assertEquals(static::$TRANSACTION_ISOLATION_READUNCOMMITTED, $level);
   +      $this->assertNotEquals(static::$TRANSACTION_ISOLATION_READUNCOMMITTED, $transaction->getTransactionIsolationLevel());
   

   This will not work for SQLite. I should go back to bitwise operator to determine the level. Might be a good idea to add that to the Trait and add an assertion.

Looks like this was filed with the rc target tag, but that seems wrong? https://groups.drupal.org/node/484788

I don't know if that still applies, but catch added it so I think it falls under "other issues at committer discretion" for performance/stability reasons.

For a long time we've had this in the Drupal 7 Driver for Drupal (and it is also available in the D8 port).

The current DTBNG should be extended to consider 2 things:

- Allow to specifiy a desired isolation level when starting a transaction
- Allow to specify how transaction nesting should work when nesting transactions

I did a mimic of the .Net approach to transaction management in the MSSQL driver.

To the proposed patch I have a few weak points... (just made a quick read of the patch and remembered the work I did for MSSQL):

- There is no concept or treatment for default isolation levels. Databases can have different isolation levels by default that can be setup by the user.

- The DSN used to connect to the database can have a transaction level defined in it. This one can be the same or differ from the default transaction level configured in the database.

- Default transaction levels need to be restored after transaction commits and rollbacks, I can't see this in the current implementation. Code outside a transaction is also - soft of - transactional and can under some circumstances be affected by the ambient transaction mode.

- Nested transactions have not been taken into account. We should add flexibility on to how nested transactions work, and acknowledge that isolation levels might collide depending on how this nesting behaviour works. This is what I did in the MSSQL driver:

• Required: A transaction is required by the scope. It uses an ambient transaction if one already exists. Otherwise, it creates a new transaction before entering the scope. This is the default value.
• RequiresNew: A new transaction is always created for the scope.
• Supress: The ambient transaction context is suppressed when creating the scope. All operations within the scope are done without an ambient transaction context

You can choose how nesting should work when starting a transaction, the default is Required.

- It would be good to somewhat fix the implicit commit behaviour of transactions in Drupal once and for all, even if it had to be explictly requested to not break BWC. Implicit commits are a danger, every other thing out there is using implicit rollbacks. This is also implemented in the MSSQL driver for Drupal.

I'd love to finally see this fixed to get some decent transaction management in Drupal :)

- There is no concept or treatment for default isolation levels. Databases can have different isolation levels by default that can be setup by the user.
- The DSN used to connect to the database can have a transaction level defined in it. This one can be the same or differ from the default transaction level configured in the database.

It's already possible for a database administrator to set the global isolation level or for it to be configured in the DSN as a session isolation level. We want the database layer to change what the database admin or user configured so that Drupal doesn't crash or cause data loss because of the global or session configuration.

@davidgarcia, can you clarify what you mean with regard to this issue?

- Default transaction levels need to be restored after transaction commits and rollbacks, I can't see this in the current implementation. Code outside a transaction is also - soft of - transactional and can under some circumstances be affected by the ambient transaction mode.

It looks like this is dependent on SQL implementation:

• MySQL: all subsequent transactions use session isolation level.
• PostgreSQL: all subsequent transactions use global or session isolation level.
• SQLite: shared cache is reset on commit if pragma changed allowing other database connections to use the new pragma setting. I think that this driver should not implement setting isolation level to read uncommitted via pragma.
• Oracle: all subsequent transactions use session isolation level.
• SQL Server 2008: isolation level is set one time for a connection until explicitly set to a different one unless it is used in a stored procedure in which case it restores afterward.

So I think that the driver for sqlite and sqlsrv will need to implement behavior as necessary to restore isolation level (if supporting isolation levels).

- Nested transactions have not been taken into account. We should add flexibility on to how nested transactions work, and acknowledge that isolation levels might collide depending on how this nesting behaviour works. This is what I did in the MSSQL driver:

Required: A transaction is required by the scope. It uses an ambient transaction if one already exists. Otherwise, it creates a new transaction before entering the scope. This is the default value.
RequiresNew: A new transaction is always created for the scope.
Supress: The ambient transaction context is suppressed when creating the scope. All operations within the scope are done without an ambient transaction context

I think this is out of scope of the issue, and isolation level testing should follow ept/hermitage tests as well as the current tests in Drupal\system\Tests\Database\TransactionTest with regard to nesting.

- It would be good to somewhat fix the implicit commit behaviour of transactions in Drupal once and for all, even if it had to be explictly requested to not break BWC. Implicit commits are a danger, every other thing out there is using implicit rollbacks. This is also implemented in the MSSQL driver for Drupal.

Agreed, but Drupal's installer depends on implicit commits because it is ok to send queries when a table may or may not exist in a transaction and then try to create them in the same transaction. :( I'm not sure if changing that behavior should be in scope of this issue.

PostgreSQL allows to set the transaction level for the transaction itself, and handles the reset itself (e.g. START TRANSACTION ISOLATION LEVEL [LEVEL]) and if I remember correctly, Oracle allows you to the same. Actually any SGBD if is fully ACID compliant should allow you to do the same. MySQL is fundamentally broken by design, so I guess that trying to fix it at the software level won't be an easy task, and should be a MySQL-only fix.

But in order to make things right, I think that the transaction level should be exposed and setable at the DBTNG transaction object instance creation time and chosen by the developer, and documentation should warn that MySQL does not handle this by itself and might be error prone. Then, the fix could be done in the MySQL-specific transaction class, the only thing we'd need is to change the session transaction level when it starts, and reset it when it when it rollbacks or commits. Thanksfully PHP being stateless and because it doesn't do any threading, it's not possible for any other thread or concurrent access to use the same session, so it would work.

I can see one only use case where changing the session transaction level for a single transaction would cause damage and it is when the software over-uses yield and yield from in order to provide asynchronous coroutines to emulate degraded pseudo threaded code (which today isn't what Drupal does, since yield from is PHP 7 only).

The right transaction level should always be explicitly set by the developer depending on how safe should be transaction at the moment he does write his piece of software, and leave alone the global context and other non-related code alone, if I remember correctly my hardcore Oracle and database lessons from when I was a student.

I think this is out of scope of the issue, and isolation level testing should follow ept/hermitage tests as well as the current tests in Drupal\system\Tests\Database\TransactionTest with regard to nesting.

Agreed, but Drupal's installer depends on implicit commits because it is ok to send queries when a table may or may not exist in a transaction and then try to create them in the same transaction. :( I'm not sure if changing that behavior should be in scope of this issue.

Considering that I have all this already implemented in the MSSQL driver - and tested in production for quite a while now - and that we all know what happens to open source problems - the more niche it becomes, the less chance it will ever get done even if an individual tries to push it forward - and seeing there is real interest in properly fixing transaction once and forever in DTBNG and that we've got real code working, this might be the chance to get this done.

I'll be trying to port the MSSQL implentation to core and see what the result looks like.

I'll be widening the scope of the patch to:

• Allow to set the isolation level when starting a transaction
• Allow to set the the nesting behaviour when starting a transaction
• Allow to choose between implicit commits or implicit rollbacks when starting a transaction
• Allow to define the default isolation level in settings.php or something similar, so that even though administrators can change this at a database level, we have a Drupal default for non database admins.

This is more or less what I had in mind, ported from the MSSQL driver. No tests yet.

I just tested that it at least passes install in MySQL, let's see what the test bot thinks about it.

$this->connection->query('SET TRANSACTION ISOLATION LEVEL ' . $level_string);
Such code should not be generic, drivers that supports START TRANSACTION ISOLATION LEVEL ... should use native feature instead of application (Drupal) side code to emulate it.

Allow to choose between implicit commits or implicit rollbacks when starting a transaction

This is a completely different feature request than improving transaction isolation support with vastly more tests and core functionality that need to be changed. I do not agree at all with adding this to the scope of this issue.

This is more or less what I had in mind, ported from the MSSQL driver. No tests yet.

I think tests should be written first so that we know what part of the spec we're aiming for. The tests are probably the most complex part of this.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -333,6 +335,62 @@ protected function popCommittableTransactions() {
+    $level_string = $this->connection->query("SELECT @@tx_isolation")->fetchField();

From #19, this is not going to work when inside of a transaction. MySQL does not provide a way to get the current transaction isolation level, only the session or global setting. :(

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -415,4 +415,11 @@ public function getFullQualifiedTableName($table) {
+    throw new \Exception("Not implemented");

Should be DatabaseWrapperException, but I don't think this should throw an exception. It should return the SERIALIZABLE or READ UNCOMMITTED (if pragma is set).

I did some bugfixes, hope this one does not annoy the testbot so much.

$this->connection->query('SET TRANSACTION ISOLATION LEVEL ' . $level_string);
Such code should not be generic, drivers that supports START TRANSACTION ISOLATION LEVEL ... should use native feature instead of application (Drupal) side code to emulate it.

Because we are using PDO, the prefered way of starting a transaction is PDO::beginTransaction() - instead of issuing a START TRANSACTION ... - that does not allow to specifiy the isolation level when being called.

I remember having issues when doing the MSSQL implementation because I wanted to bypass PDO::beginTransaction() in favor of issuing a START TRANSACTION... statement and after lots of trouble I remember reading somewhere that if you are using PDO you are expected to use PDO related transaction functions.

If we use PDO and want to start a transaction with a specific isolation level this is how I believe it should be done and how it is done in the proposed patch:

1. Change the ambient transaction isolation level (SET TRANSACTION ISOLATION LEVEL .... ) only if it is different from the ambient transaction isolation level
2. Call beingTransaction()
3. Do our stuff
4. call PDO::commit() or PDO::rollback()
5. Restore the ambient transacction level - if it was different from the one we specified we are using

On the other hand, if we intend to use START TRANSACTION ISOLATION LEVEL... statements then we cannot consume PDO's transaction related functions (commit, rollback, beginTransaction)

Something that I want to insist is on the fact the the ambient isolation level has an impact on application performance. Statements that are not in a transaction also have an isolation level - that determines how they interact with data being dealt with inside active transactions.

Not in the patch, but it would be interesting to determine what is the best ambient isolation level for Drupal and explictly tell the drivers to issue a DSN with that isolation level, and provide means for the user to override this, maybe in settings.php.

Why does the testbot insist in running Sqlite code if I only want to test MySQL? :(

Will have to fix the Sqlite implementation I guess....

From #19, this is not going to work when inside of a transaction. MySQL does not provide a way to get the current transaction isolation level, only the session or global setting. :(

Right... I made sure that this can only be run when there is no active transaction. I am only using this to obtain the initial ambient transaction so that it can be restored.

This is a completely different feature request than improving transaction isolation support with vastly more tests and core functionality that need to be changed. I do not agree at all with adding this to the scope of this issue.

Can be discussed... I believe all these changes are inextricably intertwined, but of course they could be scoped independantly. Just let me work on these as a whole, in the end it's just about removing stuff or going back to the original patch in #16.

I think tests should be written first so that we know what part of the spec we're aiming for. The tests are probably the most complex part of this.

I don't believe in test driven development for architectural things.... test driven development is to get things done fast by reducing scope as much as possible... but won't fit very well for architectural things that need a complete wide and complete vision of what is wrong and what needs to be right.

So I've writen quite some tests... not all finished... I had to modify the Connectionclass :( to be able to have two active independant connections in order to test transaction behaviour. ¿Is there any other way of getting two independent connection instances for testing purposes?

The patch and tests are not 100% finished, but very close to what should be a definitive implementation if we opt to go for the extended scope, and if not most of the code is still valid and reusable.

I'm not sure on how to write the repeatable read test, one of the connections should get locked until the other is finished, but without multithreading testing this will be a headache.

Back to @punard comment in #30, my answer is in #32.

I just want the test bot to run now... :(

This is a completely different feature request than improving transaction isolation support with vastly more tests and core functionality that need to be changed. I do not agree at all with adding this to the scope of this issue.

I think that the issue problem can not be solved without this. If a specific MySQL transaction level does not work for some code paths, and another for some other, then the whole transaction handling logic needs to be changed.

There you go, everything completed with full test coverage.

Backwards compatibility should be 100% guaranteed.

Now it's time to get the green light with the test bot with all database engines.

Getting this right is more difficult than what I was expecting :(

And this should fix Sqlite's crippled transaction support. And the patch gets bigger and bigger :(

And I hope to get two green lights with this one...

And this should fix postgre issues too.

Let's see what the test bot thinks about this.

Jackpot! This is ready for real review and to decide if scope got too much out of hand.

Bumping to 8.2.x.

Rerolling the patch... there's already a core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php file, only it extends DatabaseTestBase instead of KernelTestBase. So this file is missing that file.

I believe the last patch is overlapping with:

#2605284: Testing framework does not work with contributed database drivers

That one should be fixed first, then this one rerolled.

FYI: MySQL made READ-COMMITTED its new default in version 5.7.

@Wim no, they didn't. According to docs REPEATABLE READ is the default for both:

• MySQL 5.7
• MySQL 8.0
• MariaDB

As 9.0 requires 5.7 as minimum this issue only about 8.x

This related issue adds a transaction paramater, which sounds quite reasonable.