Race conditions with lock/cache using non-database 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.

Opened a new branch and MR (MR!8762) to try and introduce a minimal interface, common to both Connection and NonTransactionalConnection. The idea is to use this interface where we can inject both.

MySQL passes tests, but SQLite and PostgreSql fail quite spectacularly

Added a comment on the alternative MR. My feeling is that we should add the non-transactional class now, which solves a real bug (or two) and move adding a connection interface to a new issue. I certainly did consider the question of an interface for handling this, when pursuing my solution, but it would be a non-trivial deprecation dance to get it done overall. And as you have found, there are wrinkles when considering multiple drivers/db types.

Looking a bit at SQLite docs, https://www.sqlite.org/wal.html and https://www.sqlite.org/lockingv3.html, it looks to me that SQLite cannot perform a DML statement in the non-transactional connection while an open transaction in the normal connection is holding the flushing of a DML statement. For example: INSERT in nonTransactionConnection while a transaction is open in Connection, and an INSERT is in the transaction --> Database Locked exception.

Could not find a way to circumvent this, so the problem here is bigger I am afraid - and do not think depends on the MR.

On the interface - I added the open() method, but in fact it could even be empty and just be there to indicate the commonality between Connection and NonTransactionalConnection; also, we do not necessarily need to change everywhere, only where we could have one of the two alternative classes.

Thanks for the pointer on SQLite - that's unfortunate. As with a few other issues I've been working on lately, it seems this is another area where SQLite isn't really an equally-qualified production DB for Drupal if you're using features that are well-supported in other relational DBs. The test failures re: a locked DB certainly prove out the failure mode you're describing.

The only real workaround on this is to just keep the current behavior with SQLite, that is, don't use the new non-transactional connection. That would involve overriding these services for SQLite (which is already supported in all but one of the services involved - that being queue). I don't think this is a blocker, in so far as we can't not fix the related bugs just because SQLite can't hang.

At first glance, the PGSQL failure seems a little stranger. This is due to a difference in how MySQL and PgSQL handle transaction isolation. Basically, MySQL gives each transaction its own snapshot for reads and writes, while PgSQL allows select queries inside a transaction to see changes committed since the transaction opened. I updated the test to reflect this... it doesn't actually matter, but the test was written too strictly to pass for both MySQL and PgSQL.

Re: the interface question, I'm open to that change, but haven't incorporated it yet into this MR. I think it's better to either have a complete interface or an empty one. That said, I'm not sure typehinting both the non-transactional and transactional connections with the same interface gets us much, because you likely want to use the non-transactional connection sparingly. I'm concerned about situations where you'd get a non-transactional connection passed where it's not appropriate. At least currently we explicitly have to state that it's acceptable.

So I chased my tail a bit on sqlite-specific stuff, namely that I forgot to put the dblog override in a service provider because otherwise it registers itself as a logger when dblog module isn't loaded, and that breaks things.

I also discovered that you can't currently do a backend-override on a lazy service - see 🐛 Lazy services (backed by proxy classes) lose their tags, e.g. `backend_overridable` Active - I'm including a fix in this issue but if it needs to be broken out into a separate one... then at least there's a separate issue. Oy vey.

SQLite tests failing in a new way but much closer. Will pick up tomorrow.

In the alternate MR, made the interface empty. So we can typehint with the interface where we can accept both Connection and NonTransactionalConnection, or the more specific class where needed.

Also, tried a different approach to mark a db driver allowing or not concurrent non-transactional connections, which does not involve services.

Finally - diverging MySql from SQLite here is starting to scare me a bit, we would end up everywhere in very different db state results depending on whether the db supports or not the concurrent non-transactional connection...

Recapping my debugging this morning.

I think backend-overriding queue.database has been broken since at least 📌 Convert QueueFactory to use a service locator Fixed introduced a service locator.

Here's what happens:

• BackendCompilerPass adds an alias for the overridden service's ID pointing at the override service.
• The service locator is looking for services tagged with queue_factory, which now results in an empty ServiceLocator passed to the queue service.
• CoreServiceProvider does tag instances of QueueFactoryInterface with queue_factory, but that doesn't appear to extend to services defined by modules, including sqlite. I think that it probably should but I'm not sure why it isn't working. Probably an issue with order of operations in the service providers and passes.
• Manually tagging sqlite.queue.database, the backend-override service, as a queue_factory results in it showing up in the service locator instance passed to the queue service, but it's named sqlite.queue.database, not queue.database, which is the hard-coded default that is requested. Service locators aren't aware of aliases.

Even if the override service got the tag automatically, that doesn't change the fact that queue is requesting queue.database as a default, which is an alias. I think the answer is in how BackendCompilerPass registers the overrides. In cases where the service is requested directly from the "main" container, this all works fine. But laziness causes this go to awry.

I'll see about opening a separate issue to track this, but it is a hard blocker for the sqlite carve-out we need here.

I'm ambivalent on a flag that indicates the specific nature of the read-committed implementation. It seems like it's mostly only really useful for tests, but perhaps a public method for indicating this behavior would help some power-user developers. I'm tempted to not add to the API surface of the connection for something that we can address in tests - it's only really a MySQL vs. Postgres difference. But I don't care much either way, there are bigger issues uncovered here.

diverging MySql from SQLite here is starting to scare me a bit, we would end up everywhere in very different db state results depending on whether the db supports or not the concurrent non-transactional connection

I don't think we really have any alternative. I would suggest couching it as "SQLite diverging from MySQL/MariaDB/Postgres," and not the other way around. As explored in detail over at ✨ Add "json" as core data type to Schema and Database API Needs work , SQLite is divergent from the other, non-file based drivers in a number of ways and I imagine we will continue to root out these differences as Drupal becomes more advanced and modern under the hood. I think there is a loose but growing consensus that running Drupal on SQLite in production will mean accepting these limitations or trade-offs. If we insist on 100% parity, it means that we sacrifice innovation for a small minority of implementations. I don't think we should do so.

The failure on SQLite is 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review . Seems like there is some sort of regression with PgSQL though... playing whack-a-mole. This issue has turned into fixing what appears to be a very-not-stable backend-override API more than the original goal. Gotta fix all that though, at this point.

Looks like all tests are passing now, next step is to incorporate some of the changes from the alternative MR.

Hacked a bit more on this tonight and am more happy with where this is going. Kind of a compromise between the two MRs.

Per #95:

In the alternate MR, made the interface empty. So we can typehint with the interface where we can accept both Connection and NonTransactionalConnection, or the more specific class where needed.

I toyed with this but eventually landed on a fuller interface. Reason being, the non-transactional connection can "do" most everything the wrapped/transactional connection can, so let's make it a first-class citizen. I think it's also good to move toward an interface for Connection, instead of treating the abstract class as a makeshift interface. I am not wed to this but the interface does help with static analysis way more than using __call().

Also, tried a different approach to mark a db driver allowing or not concurrent non-transactional connections, which does not involve services.

I pulled the flag into my MR and the new interface. I kept the service override, however, since SQLite is still the outlier here and all the services in question are backend-overridable. This is what that model is for, so let's use it instead of hiding the logic.

Finally - diverging MySql from SQLite here is starting to scare me a bit, we would end up everywhere in very different db state results depending on whether the db supports or not the concurrent non-transactional connection...

As discussed a bit above... I'm not sure there's really any way around this. Yes, you will get some different results with SQLite but that's no different than now, and in the vast majority of production use cases this will result in more deterministic and predictable behavior, not less. I think that's a win.