Race conditions with lock/cache, session storage - add a non-transactional database connection

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This came up in #2833539-134: SQLSTATE[40001]: Serialization failure: 1213 Deadlock → - see the comment which reaches similar conclusions to this issue (but also has at least one possibly alternative approach).

Re-rolled patch from #41.

 ╰❯ $ diff D10.0.x-nontransactional_connection-2347867-61.patch D10.1.x-nontransactional_connection-2347867-62.patch 
2c2
< index e3c1e094..f2ed065d 100644
---
> index ed531f5027..13bbd46cdd 100644
5c5
< @@ -213,7 +213,7 @@ services:
---
> @@ -220,7 +220,7 @@ services:
14,15c14
< @@ -386,6 +386,10 @@ services:
<      class: Drupal\Core\Database\Connection
---
> @@ -405,6 +405,10 @@ services:
17a17
>    Drupal\Core\Database\Connection: '@database'
25,26c25,26
< @@ -485,7 +489,7 @@ services:
<        - [setContainer, ['@service_container']]
---
> @@ -527,7 +531,7 @@ services:
>    Drupal\Core\Queue\QueueFactory: '@queue'
30a31
>    Drupal\Core\Queue\QueueDatabaseFactory: '@queue.database'
33,34c34
<      arguments: ['@request_stack']
< @@ -870,13 +874,13 @@ services:
---
> @@ -959,13 +963,13 @@ services:
51c51
< index 96ac2ee9..78cac386 100644
---
> index 96ac2ee948..78cac38694 100644
63,64c63,64
< diff --git a/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php b/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
< index e41df19c..c1145c5f 100644
---
> diff --git a/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
> index ebbf60826a..e09d8dce65 100644
75c75
< @@ -573,4 +574,54 @@ public function testQueryFailureInTransaction() {
---
> @@ -575,4 +576,54 @@ public function testQueryFailureInTransaction() {

According to the current dictionary, nontransactional is not a word.

I would argue otherwise as this makes perfect sense to those of us who know what transactional processing is in a relational database sense from a contemporary RDBMS world.

How do we go about adding nontransactional to the dictionary so that we can continue with automated testing?

You can add it to misc/cspell/dictionary.txt.

Rerolled, has tests, passes tests

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled 66 to resolve a little fuzz on the 11.x patch
the 10.1.x patch still applies cleanly.

This looks good!

Wonder if a change record could be added though for the new service database.nontransactional? If it's something that other contrib modules could leverage.

Think it could be RTBC after that.

Thanks.

@smustgrave, I have created a draft change record.

https://www.drupal.org/node/3384048 →

Thanks!

latest patch needs a reroll

Rerolled

This is a good idea, but as it is this patch is practically not doing what it's meant to do.

+++ b/core/core.services.yml
@@ -415,6 +415,10 @@ services:
+  database.nontransactional:
+    class: Drupal\Core\Database\Connection
+    factory: Drupal\Core\Database\Database::getConnection
+    arguments: [nontransactional]

This service will try to fetch a db connection with 'nontransactional' as a target, which unless it is defined in the settings.php, will fall back to the same 'default' target, see this code in Database::getConnection:

    // If the requested target does not exist, or if it is ignored, we fall back
    // to the default target. The target is typically either "default" or
    // "replica", indicating to use a replica SQL server if one is available. If
    // it's not available, then the default/primary server is the correct server
    // to use.
    if (!empty(self::$ignoreTargets[$key][$target]) || !isset(self::$databaseInfo[$key][$target])) {
      $target = 'default';
    }

So you would still doing the cache and log operations on the same default Connection object.

The test works because you are preliminarily adding the 'nontransactional' database target in

+++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
@@ -680,4 +681,55 @@ public function testConnectionDeprecations(): void {
+    // Setup a secondary database connection.
+    $connection_info = Database::getConnectionInfo('default');
+    Database::addConnectionInfo('default', 'nontransactional', $connection_info['default']);

So something will have to happen similarily in the getConnection() method if (hopefully) we won't be changing settings.php.

I also think if we open such a transaction-free connection, we should prevent it from executing startTransaction(). Could be a follow up, but IMHO it would be better to get it straight from the beginning.

Also, the IS could use some updates… the code is still D7.

OK, marking this NR.

Digging into this a bit, I discovered/decided a few things:

• Most of Drupal core expects that you'll really only have one default target and (maybe) a replica target. Non-replica targets for a database key, especially default, aren't anticipated in production and this results in a required bugfix here. Basically, the assumption was that if there were more than one target, it must be a replica, and thus we need to disable replicas during certain actions. This was discovered during a regression surfaced in testing where the session was re-started due to the replica kill switch service.
• Speaking of replicas, we use that term a lot around the DB API and it's not entirely clear that replica is a reserved word that specifically designates a replica or group of replica connection targets. It was particularly difficult to find all instances of this "DB replica" vs. other uses of the word. I think constants are a great answer to this question, so in the course of fixing above bug in adding this functionality, I extracted all these words into constants.
• I landed on a decorator pattern for the non-transactional connection, which wraps the underlying driver's connection object. As I have navigated in the course of my work on JSON data support, messing with signatures for the various connection classes and interfaces is no small feat, and there are also contrib drivers to think about. Decoration is a good solution here, but as you can see it results in a lot of boilerplate. I marked this class both final and internal, and it did still require a change broadening the potential return types of connection methods. (see the event enable and disable methods.) As I expect this to really be an internal API (there's little to no reason to override the non-transactional decorator class) I'm hoping this is sufficient. Otherwise we need to add a bunch of code to the connection classes that boils down to throwing an exception if you try and start a transaction on this connection. I don't think that's worth it.
• This also required a few tweaks to performance testing (though with no real change in performance) due to there now being two connections always in play - transactional and non.

Great idea tp use a decorator. Added some comments/ideas inline - free thoughts.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.