SQLite database locking errors cause fatal errors

Hello rfay,

can you share your research about this?

I have ubuntu 10.10 with SQLite 3.7.2, and my simple tests also shows me error you mentioned. My first impression is that in comparison to MySQL-based SQLite-based Drupal is not really usable out-of-the-box.

Regards, Dennis

Randy,

answering to your comment #3.

With 95% chances I can reproduce error like

<em>D7 SQLite<em>
Error
The website encountered an unexpected error. Please try again later.
Error messagePDOException: SQLSTATE[HY000]: General error: 5 database is locked: INSERT INTO {search_index} (word, sid, type, score) VALUES (?, ?, ?, ?); Array ( [0] => got [1] => 11 [2] => node [3] => 0.3961003861 ) in search_index() (line 721 of /var/www/d7sqlite/modules/search/search.module). 

with the following steps:

1) Ensure standard Search module is on by visiting admin/modules

2) Create a basic Page, insert in a body andy medium-size (10 .. 100 Kbytes) pure text document like Apache readme, or, in this sample, http://london.thefreelibrary.com/Burning-Daylight/2-11;

3) now you can see that at page admin/config/search/settings you have one (in my case) doc to indexing

4) initiate cron job ( visit admin/reports/status/run-cron or any other way you prefer).

5) During indexing access in other windows frontpage and/or administering pages

6) Indexing will trap with the message above. Document is marked as indexed, page admin/config/search/settings gives info that everything is indexed. However text from last doc may be unindexed - in my case the word 'looker' cannot be found but it is in text.

That's it.

I also checked most important PRAGMA of SQLite database (like 'Syncronous'), seems for me all are nicely configured.

Basically, page http://www.sqlite.org/whentouse.html especially the last item, gives a hint that probably using Sqlite with Drupal is just not a good approach. Everybody knows that Drupal generates a lot of small queries, and Apache may serve request in different thread.

@denikin, I used basically your approach but just had devel_generate create 50 nodes and then have cron index the search. Interestingly, it was the cron that ended up failing, not the page loads, which basically completed a *long* time later.

That's totally expected. A long running process that holds a transaction open will lock the database. SQLite does not handle write concurrency, at all. Setting the timeout should mitigate this so that it is not noticeable in normal usage.

+++ b/includes/database/sqlite/database.incundefined
@@ -37,19 +37,24 @@ class DatabaseConnection_sqlite extends DatabaseConnection {
+  protected $busy_timeout = 60; // Wait 10 seconds on busy database.

The comment says 10 seconds, but the code would indicate 60..

Also some of the comments on the internet seem to indicate the default, in sqlite at least, is already 60.

Powered by Dreditor.

Grepping the PHP source code, I don't think PHP is setting a default value for this.

Damien: Well, the source code is always authoritative. :) My comment above was based on http://bugs.php.net/bug.php?id=38182

Everything points to this actually working to increase the timeout duration, and yet the lines of code! They do nothing!

For a while, I tried doing clever things like wrapping the query execution code in try/catch and repeating the query if the timeout exception was thrown, but that was messy and stupid and just caused other problems.

Possibly of interest is this page in SQLite's API docs. This is the function that PDO is calling.

Cleaned-up but still non-functional patch attached. Damn.

60 seconds is 60,000 milliseconds, which would cause an overflow if your C compiler is interpreting the value as a signed int with a max value of 32,767. Alas, I tried setting the value to 30, 15, 10 and 5, and still had the same problems.

I posted a question on Stack Overflow about this, hoping that maybe someone outside the Drupal community knows the key. We'll see if anything comes of it.

Possibly related email thread - from five years ago…

WTF mode engaged! If we break transactions in the SQLite DB driver… everything works. I can run a cron task doing search indexing and open browser tabs with site content as fast as I my browser will let me. Occasionally one of the tabs will spin until the cron run finishes, but most of them just pop open instantly.

I'm afraid I don't understand the guts of PDO and/or DBTNG to understand what might be going on here. But perhaps it points us in the right direction.

#17 is not desirable.

#15 is. I don't know why it doesn't work for your particular version of PHP. I checked the PHP 5.3 source code and support for this is implemented. Could you report on the details of your PHP install?

Oh, dear, I'm not saying we should actually commit #17. Just that something about it causes things to work, so maybe it's a hint towards what we need to do.

I take it from your comment that you can't replicate this?

Here's my phpinfo() page.

Took another look at this today, but still couldn't find a solution that didn't involve disabling transactions.

I can't help but wonder if this quote from SQLite's C API documentation is relevant:

There can only be a single busy handler for a particular database connection any any given moment. If another busy handler was defined (using sqlite3_busy_handler()) prior to calling this routine, that other busy handler is cleared.

This is a hellbug.

@rfay: I think remaning is correct. Search is still the best option to reveal this bug, anybody can easily find this in thread text.

If these are merge queries that are failing, I wonder if it's a similar thing to #937284: DEADLOCK errors on MergeQuery INSERT due to InnoDB gap locking when condition in SELECT ... FOR UPDATE results in 0 rows which is due to SELECT .. FOR UPDATE as far as I can tell. Just in MySQL SELECT ... FOR UPDATE behaves differently with different transaction isolation levels, I have no idea what happens with sqlite with that though.

Just a random idea, besides upgrading to 3.7 also try http://www.sqlite.org/draft/wal.html this.

Good point; I made the same tests described in #6 with WAL on and off. With WAL you also have locking problem but chances of getting these significantly reduced.

So at a glance having WAL turned on by default is nice. Also it is cool that setting WAL is persistent - we can do it only on DB creation, no need to update connection settings.

it seems in my case the error is thrown *without any noticeable delay* (as also noted in some of the above threads).
so i dare the conjecture that this is in fact 2 issues:
* a locking timeout which appears after "too long waiting"
* some other issue like stated in SQLite "Database table is locked" errors in PDO | TechnoSophos (cited by rfay in #8) which i do not understand =;-) but has something to do with dangling database cursors. See also PHP: PDO::beginTransaction - Manual (cited by rfay in #8)

that the patches above dealing with timeout do not seem to cure anything supports this conjecture.

EDIT: i can reproduce this error here in 90% of all cases
* by viewing admin/modules for a 80-modules-site
* with *no* other concurrent request

EDIT: here is a thread that tells us "sqlite freaks out when you do a write while a read is open":
"[sqlite] Table locked when trying to delete a record whilst a cursor to":http://www.mail-archive.com/sqlite-users@sqlite.org/msg04547.html

EDIT: this is from a D7 installation! chance is that the code from #27 backported may solve this.

Yes but the SQLite backend goes really really far to try to avoid having cursors open. That's why we have DatabaseStatementPrefetch extended by DatabaseStatement_sqlite and this in execute:

    $statement = $this->getStatement($this->queryString, $args);
    if (!$statement) {
      $this->throwPDOException();
    }

    $return = $statement->execute($args);
    if (!$return) {
      $this->throwPDOException();
    }

    // Fetch all the data from the reply, in order to release any lock
    // as soon as possible.
    $this->rowCount = $statement->rowCount();
    $this->data = $statement->fetchAll(PDO::FETCH_ASSOC);
    // Destroy the statement as soon as possible. See
    // DatabaseConnection_sqlite::PDOPrepare() for explanation.
    unset($statement);

I fail to see how could we keep the statement alive for a shorter time. Does something keep the statement object alive??

thanks chx for the insight!
i see a possible gotcha if someone tries a statement, catches exception and tries next statement.

what do you think about being paranoid to destruct $statement like this (or is this done by exception?):

 $return = $statement->execute($args);
    if (!$return) {
      unset($statement);
      $this->throwPDOException();
    }

what i already notice is: it seems that the code cited in #27 is NOT in d7 and thus needs backport.
(will edit my #26)

Of course it is in D7, it's in includes/database/prefetch.inc. If you throw an exception then you leave the function scope so the local variables are automatically lost, objects are unset, we only do an unset to make it happen ASAP. The question is, does something keep the statement alive? It's untrivial to debug, add a global before the unset change it afte. and then write a class that extends the sqlite pdostatement, return that instead of the raw pdostatement and add a __destruct which logs the value of said global. debug_zval_dump won't help because the objects dont have meaningful refcounts.

I've been banging my head against this in another project and thought I'd share what I'd learnt - there is another cause for this beyond unclosed cursors...

The PDO beginTransaction() method obtains a DEFERRED lock and if the first PDOStatement you execute doesn't open a cursor (ie INSERT, UPDATE, DELETE) then it will block until the database is available. If it does open a cursor (SELECT) then it throws a General error: 5 database is locked immediately.

So this will block as expected until the DB is available:

<?php
$db->beginTransaction();
$insert->execute(array('a value'));
$select->execute();
$data = $select->fetchAll();
$db->commit();
?>

but this will throw the error if the DB is locked by another process.

<?php
$db->beginTransaction();
$select->execute();
$data = $select->fetchAll();
$insert->execute(array('a value'));
$db->commit();
?>

You can fix this by using an IMMEDIATE or EXCLUSIVE lock as opposed to the default DEFERRED so the following should work as expected:

<?php
$db->exec("BEGIN IMMEDIATE TRANSACTION");
$select->execute();
$data = $select->fetchAll();
$insert->execute(array('a value'));
$db->exec("COMMIT");
?>

Have attached my test script which gives your a good idea of what is going on - run on command line with two consoles.
There is a whole bunch of stuff about how SQLite does its low level locking here and the differences between the lock types here

A few more notes on this, more for my own reference but here is as good a place to put them as any...

The locker is the long process holding the lock of the described type. The accessor is the second
process trying to access the data via the following methods. DEFERRED is the default which you get with beginTransaction()

1. SELECT with no transaction.
2. INSERT with no transaction.
3. SELECT in accessor transaction
4. INSERT in accessor transaction
5. SELECT then INSERT in accessor transaction
6. INSERT then SELECT in accessor transaction

No blocking

Call returns almost immediately, presumably from the existing data as the locker hasn't commited the insert yet

Blocking

Accessor blocks up to PDO::ATTR_TIMEOUT in seconds. If still not able to access database then accessor throws Error 5: database is locked.

Deadlock

Deadlocks both processes. Whichever process reaches their PDO::ATTR_TIMEOUT first throws Error 5: database is locked
and sometimes so does the other process as the lock is not correctly released

Error locked

Accessor throws Error 5: database is locked at the INSERT.

1. will block and succeed if there has been no previous transaction by the accessor and throw the error if there was a previous transaction (!)
2. (SELECT before INSERT) throw a further Error 5: cannot rollback transaction - SQL statements in progress even if the SELECT cursor is closed - the only solution to this would be to try/catch the rollback untill the locker releases otherwise the connection is left in an inconsistent state. Don't do a read followed by an insert in a transaction
3. (INSERT before SELECT) can be rolled back as expected.

Once you can explain the odd results where something that shouldn't succeed does and vice versa then the results table fits with what you'd expect: If the locker has anything other than an EXCLUSIVE then the accessor can read fine - inside or outside a standard deferred lock. Once the accessor tries to write to the database then it throws an Error.

Unfortunately non of this fits with your current nested transaction model, except that if you use an EXCLUSIVE lock for your long running cron actions then the web requests will time out according to PDO::ATTR_TIMEOUT in a predictable way as opposed to random errors.

One additional thing is that any open cursor, even a fetchAll(), on any process will cause a rollback to fail so that is another random thing that complicates things.

This is also slightly at odds with my previous post as the first example is DEFERRED/DEFERRED so should have thrown an error but didn't in testing - go figure! Again, have attached test scripts for reference.

thank you tris for sharing your findings! this really sounds like sqlite locking is a major PITA right now.

i only see 2 sinple ways to approach this.
the first, always setting exclusive locks, sounds like a performance nightmare.
the other: if sqlite throws "busy", sleep and retry.
surely doing polling this way should not be our job but a snippet from your second link above suggests sqlite wants us to:

An attempt to execute COMMIT might also result in an SQLITE_BUSY return code if an another thread or process has a shared lock on the database that prevented the database from being updated. When COMMIT fails in this way, the transaction remains active and the COMMIT can be retried later after the reader has had a chance to clear.

If it was just COMMIT that threw the SQLITE_BUSY that would be easy enough to adapt to, but it can be any read, write or transaction command. Exclusive locks would make SQLite unusable in anything other than testing, and even then requests for generated resources e.g. css could cause problems.

The only solution I can see is to wrap both PDO and PDOStatement in proxy classes, magic method most of the stuff through to the underlying objects and try/catch/sleep the PDO exec, query, beginTransaction and commit methods and the PDOStatement::execute() method. I did something similar for a Logging PDO class recently and it worked well...

of course we have to address explicit *and* implicit commits.
what you outline is the same i thought of but did not have opportunity yet to look into the code.

I cannot reproduce any of this. The following works as expected for me (PHP 5.3.13 / SQLite 3.7.7.1 from OS X Mountain Lion):

$db = new PDO('sqlite:test.db');
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$db->setAttribute(PDO::ATTR_TIMEOUT, 10);

$db->exec('CREATE TABLE IF NOT EXISTS locktest (item TEXT)');

echo "BEGIN";
$db->exec("BEGIN");
echo "SELECT";
$db->exec("SELECT 1");
echo "INSERT";
$db->exec("INSERT INTO locktest(item) VALUES('test')");
echo "SLEEP";
sleep(5);
echo "COMMIT";
$db->exec("COMMIT");

If you launch two of those, I see the expected sequence:

T1: BEGIN SELECT INSERT SLEEP
T2: BEGIN SELECT INSERT (waiting for the lock)
T1: COMMIT
T2: SLEEP COMMIT

If I change the BEGIN into a BEGIN IMMEDIATE, I also see the expected:

T1: BEGIN SELECT INSERT SLEEP
T2: BEGIN
T1: COMMIT
T2: SELECT INSERT SLEEP

Anyway, we DO want to switch to BEGIN IMMEDIATE in Drupal (it should be as easy as overriding PDO::beginTransaction(): our MergeQuery implementation is currently broken on SQLite because of deferred transactions. We wrongly assumed that once you are in a transaction, a write lock has been acquired (which is false with deferred transaction) and thus we decided that we don't need to support SELECT FOR UPDATE queries.

i can confirm damiens tests on my sqlite3.7.7 linux box.

if i change the script so that sleep > timeout then i get said error after the timeout.

no occurence of my earlier reported immediate errors.

Quick comment on Damiens test - you are not getting an error because your select is not actually hitting the database. Tested the following with PHP 5.3.10 / SQLite 3.7.9 and you get an Error 5 database locked as at the INSERT statement for the second process.

$db = new PDO('sqlite:test.db');
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$db->setAttribute(PDO::ATTR_TIMEOUT, 10);

$db->exec('CREATE TABLE IF NOT EXISTS locktest (item TEXT)');

echo "BEGIN";
$db->exec("BEGIN");
echo "SELECT";
$db->exec("SELECT item FROM locktest LIMIT 1");
echo "INSERT";
$db->exec("INSERT INTO locktest(item) VALUES('test')");
echo "SLEEP";
sleep(5);
echo "COMMIT";
$db->exec("COMMIT");

i can also confirm #37.

> Quick comment on Damiens test - you are not getting an error because your select is not actually hitting the database.

GEE, i was confirming without thinking...

So conclusion: seems we are back at the verdict of #32-34.

I can confirm. The first statement hitting a lock results in an immediate "database is locked" exception when using deferred transactions (that smells like a bug in SQLite big time).

Using BEGIN IMMEDIATE results in the expected behavior. So it seems like just using immediate transactions would solve the problem in Drupal (it is a good idea anyway, as I explained in #35).

here is a patch for this to review.
also crosslinking #1764454: Erroneous static PDO calls in DB driver.

+  public function beginTransaction() {
+    return $this->exec('BEGIN IMMEDIATE') !== FALSE;
+  }

Thanks for the patch, but this doesn't seem correct. PDO::exec() returns the number of affected rows, so it should always return 0 here. We can just drop the test and always return TRUE. Contrary to the documentation, it seems like most drivers will just raise an exception if something goes wrong with the transaction.

Hello Damien,

I set out to correct/improve Alex's patch (#40) as you have suggested, but after a careful investigation, I am convinced that Alex's patch is already perfect as is, and I cannot make it any better than it already is.

PDO::exec() usually returns the number of affected rows, but it does return Boolean FALSE instead when the execution fails. The official documentation on http://www.php.net/manual/en/pdo.exec.php mentions this, kind of like an after-statement, in the Warning section:

Return Values

PDO::exec() returns the number of rows that were modified or deleted by the SQL statement you issued. If no rows were affected, PDO::exec() returns 0.

Warning

This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.

The following example incorrectly relies on the return value of PDO::exec(), wherein a statement that affected 0 rows results in a call to die():

$db->exec() or die(print_r($db->errorInfo(), true));

Note that, in his patch, Alex was careful to use the !== (Not identical) operator to test specifically for Boolean FALSE, not any 0.

And I don't think we can always rely on an exception being raised when there is an error. For example, without $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);, no exception is raised if "BEGIN IMMEDIATE" fails, but a Boolean FALSE is indeed raised, exactly matching the behaviour of the original PDO::beginTransaction().

That said, I decided to try to test the patch in D8 to see if it actually solves the database lock problem and to prove my theory above. Initially, I thought the patch was working perfectly. However, it turns out that this overriden beginTransaction() was never called!

1. Drupal\Core\Database\Driver\sqlite\Connection->pushTransaction() checks for savepointSupport, which is TRUE for SQLite >= 3.6.8, and proceeds to call parent::pushTransaction(), never calling the new beginTransaction()...
2. Even for SQLite << 3.6.8, pushTransaction() calls PDO::beginTransaction()... Oh, #1764454: Erroneous static PDO calls in DB driver is probably an attempt to fix this.

So, I am stuck. Oops...

Tagging

Triaging this issue as part of Drupalcon LA. This is still an issue, is still a major and should be fixed. Marking that patch needs a re-roll and some manual testing.

Sprinting at Drupal North, I'm going to attempt to re-roll this.

Attaching re-rolled patch.

The reroll is good.

Axel.rutz (#40) and Anthony Fok (#42) agree on the chosen solution.

What I miss is some tests so we can confirm that the solution fixes the problem.

Putting it back to SQLite db driver because that is the base problem and not the ajax-system.

The approach of the current patch can not work anymore because we split the db connection object from PDO in #1953800: Make the database connection serializable, so we need another class that extends PDO specifically for SQLite.

Also, if we override PDO::beginTransaction(), we also need to override commit() and rollBack() otherwise PDO is not aware of our manual initialization of a transaction.

I hope that this patch will also help with #2508567: DrupalCI SQLite random failures, but we need to run the full test suite on DrupalCI with the patch attached for confirmation.

Patch testing on sqlite works on drupalci. I would resubmit the patch to be able to do that.

We already know from a previous round of testing that this patch does not influence test pass rate on DrupalCI, but it does slow everything down dramatically. Needs some investigation to find out why.

Lets see how much slower the testbot get with this patch. The patch is a reroll of the patch from comment #40.

I think that the locking problems with SQLite will be gone if we fix #2348137: Enable WAL journal mode by default for SQLite database.

As #25 said wal does not solve the issue

Straight reroll to 8.4.x.

Brought the patch in line with our current standards by removing the file docblock.

We were having locking errors on our test infrastructure when running BrowserTestBase tests using SQLite, and this patch solves it completely.

So this needs some test coverage, but how do we approach that? Is there a reliable way to trigger the errors?

I tried the script from #37 but it works without errors on PHP 7.1.10 and SQLite 3.20.1.

Just verified it, the patch still applies correctly to 8.5.x.

Updated the issue summary.

The Entity Browser project has a consistent failure with SQLite with this specific error message since August 2017:
- https://www.drupal.org/node/1943336/qa
- https://www.drupal.org/pift-ci-job/1005071

I have not investigated if the patch solves the issue

@idebr perhaps related to work going on in #2878119: Whether queued or not, update the media thumbnail and metadata before beginning the entity save database transaction that try to correctly tackle locking issues when it try to grab meta from external sources while there is a write lock etc. Worth a read and what changed with the temp. change before it got committed to 8.6.x

This bug is a hard blocker for #2966607: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock, because QuickStartTest fails 100% of the time due to this.

Is this really hard blocker?

[#59] solves the issues I've been having locally.

According to #2966607-81: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock, the latest patches over there not just no longer blocked on this, they actually seem to *fix* this problem as well.

Whoever relies on this, can you test the latest patch from there and confirm that?

I don't know exactly why this happens, but by moving the cachetags table changes to run outside of the transaction, I guess there are less complex things to resolve and doing it before the transaction ends made it even worse.

Thanks for the suggestion @Berdir, but I've just tested it and the patch in this issue fixes the problem whereas the one in #2966607: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock throws the locked errors still.

@Berdir and @pcambra

Tested the following on Drupal 8.7.3 with a custom entity
sqlite patch 1120020-59.patch
cachetag patch 2966607-105.patch

In my testing I found that the sqlite patch helped significantly (we have other issues going on with Drupal core's locking), but the cachetag patch continued to deadlock in the same way and made no difference.

Environment: Drupal 8.7.3
Test: Concurrent creation of custom entity and file uploads via REST

Error with cache tag patch
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 5 database is locked: INSERT OR REPLACE INTO {cache_entity} (cid, expire, created, tags, checksum, data, serialized) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => values:file:784 [:db_insert_placeholder_1] => -1 [:db_insert_placeholder_2] => 1563428513.903 [:db_insert_placeholder_3] => entity_field_info file_values [:db_insert_placeholder_4] => 752 [:db_insert_placeholder_5] => O:23:"Drupal\file\Entity\File":28:{s:9:"*values";a:11:{s:3:"fid";a:1:{s:9:"x-default";s:3:"784";}s:4:"uuid";a:1:{s:9:"x-default";s:36:"c5b74236-167e-435f-b6f1-1dd496958566";}s:8:"langcode";a:1:{s:9:"x-default";a:1:{i:0;a:1:{s:5:"value";s:2:"en";}}}s:3:"uid";a:1:{s:9:"x-default";s:2:"38";}s:8:"filename";a:1:{s:9:"x-default";s:6:"41.bin";}s:3:"uri";a:1:{s:9:"x-default";s:55:"private://foldersharefiles/0000/0000/0000/0000/0784.bin";}s:8:"filemime";a:1:{s:9:"x-default";s:24:"application/octet-stream";}s:8:"filesize";a:1:{s:9:"x-default";s:6:"439808";}s:6:"status";a:1:{s:9:"x-default";s:1:"0";}s:7:"created";a:1:{s:9:"x-default";s:10:"1563428513";}s:7:"changed";a:1:{s:9:"x-default";s:10:"1563428513";}}s:9:"*fields";a:0:{}s:19:"*fieldDefinitions";N;s:12:"*languages";N;s:14:"*langcodeKey";s:8:"langcode";s:21:"*defaultLangcodeKey";s:16:"default_langcode";s:17:"*activeLangcode";s:9:"x-default";s:18:"*defaultLangcode";s:2:"en";s:15:"*translations";a:1:{s:9:"x-default";a:1:{s:6:"status";i:1;}}s:24:"*translationInitialize";b:0;s:14:"*newRevision";b:0;s:20:"*isDefaultRevision";b:1;s:13:"*entityKeys";a:6:{s:6:"bundle";s:4:"file";s:2:"id";s:3:"784";s:5:"label";s:6:"41.bin";s:8:"langcode";s:2:"en";s:4:"uuid";s:36:"c5b74236-167e-435f-b6f1-1dd496958566";s:5:"owner";s:2:"38";}s:25:"*translatableEntityKeys";a:1:{s:8:"langcode";a:1:{s:9:"x-default";s:2:"en";}}s:12:"*validated";b:0;s:21:"*validationRequired";b:0;s:19:"*loadedRevisionId";N;s:33:"*revisionTranslationAffectedKey";s:29:"revision_translation_affected";s:37:"*enforceRevisionTranslationAffected";a:0:{}s:15:"*entityTypeId";s:4:"file";s:15:"*enforceIsNew";N;s:12:"*typedData";N;s:16:"*cacheContexts";a:0:{}s:12:"*cacheTags";a:0:{}s:14:"*cacheMaxAge";i:-1;s:14:"*_serviceIds";a:0:{}s:18:"*_entityStorages";a:0:{}s:12:"*isSyncing";b:0;} [:db_insert_placeholder_6] => 1 ) in Drupal\Core\Entity\ContentEntityStorageBase->setPersistentCache() (line 1049 of /var/www/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

Error with D8.7.3 without any patch
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 5 database is locked: INSERT OR REPLACE INTO {cache_entity} (cid, expire, created, tags, checksum, data, serialized) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => values:file:749 [:db_insert_placeholder_1] => -1 [:db_insert_placeholder_2] => 1563428205.317 [:db_insert_placeholder_3] => entity_field_info file_values [:db_insert_placeholder_4] => 752 [:db_insert_placeholder_5] => O:23:"Drupal\file\Entity\File":28:{s:9:"*values";a:11:{s:3:"fid";a:1:{s:9:"x-default";s:3:"749";}s:4:"uuid";a:1:{s:9:"x-default";s:36:"30ccdd21-6167-4011-9372-74d06bd2177a";}s:8:"langcode";a:1:{s:9:"x-default";a:1:{i:0;a:1:{s:5:"value";s:2:"en";}}}s:3:"uid";a:1:{s:9:"x-default";s:2:"37";}s:8:"filename";a:1:{s:9:"x-default";s:6:"34.bin";}s:3:"uri";a:1:{s:9:"x-default";s:55:"private://foldersharefiles/0000/0000/0000/0000/0749.bin";}s:8:"filemime";a:1:{s:9:"x-default";s:24:"application/octet-stream";}s:8:"filesize";a:1:{s:9:"x-default";s:7:"1587648";}s:6:"status";a:1:{s:9:"x-default";s:1:"0";}s:7:"created";a:1:{s:9:"x-default";s:10:"1563428205";}s:7:"changed";a:1:{s:9:"x-default";s:10:"1563428205";}}s:9:"*fields";a:0:{}s:19:"*fieldDefinitions";N;s:12:"*languages";N;s:14:"*langcodeKey";s:8:"langcode";s:21:"*defaultLangcodeKey";s:16:"default_langcode";s:17:"*activeLangcode";s:9:"x-default";s:18:"*defaultLangcode";s:2:"en";s:15:"*translations";a:1:{s:9:"x-default";a:1:{s:6:"status";i:1;}}s:24:"*translationInitialize";b:0;s:14:"*newRevision";b:0;s:20:"*isDefaultRevision";b:1;s:13:"*entityKeys";a:6:{s:6:"bundle";s:4:"file";s:2:"id";s:3:"749";s:5:"label";s:6:"34.bin";s:8:"langcode";s:2:"en";s:4:"uuid";s:36:"30ccdd21-6167-4011-9372-74d06bd2177a";s:5:"owner";s:2:"37";}s:25:"*translatableEntityKeys";a:1:{s:8:"langcode";a:1:{s:9:"x-default";s:2:"en";}}s:12:"*validated";b:0;s:21:"*validationRequired";b:0;s:19:"*loadedRevisionId";N;s:33:"*revisionTranslationAffectedKey";s:29:"revision_translation_affected";s:37:"*enforceRevisionTranslationAffected";a:0:{}s:15:"*entityTypeId";s:4:"file";s:15:"*enforceIsNew";N;s:12:"*typedData";N;s:16:"*cacheContexts";a:0:{}s:12:"*cacheTags";a:0:{}s:14:"*cacheMaxAge";i:-1;s:14:"*_serviceIds";a:0:{}s:18:"*_entityStorages";a:0:{}s:12:"*isSyncing";b:0;} [:db_insert_placeholder_6] => 1 ) in Drupal\Core\Entity\ContentEntityStorageBase->setPersistentCache() (line 1049 of /var/www/html/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

Hi here,
Patch # 59 works like a charm in our case

I see issue status is Postponed (maintainer needs more info). If you are looking for a way to reproduce the issue you can do it "easily" that way :

1. git clone https://github.com/skilld-labs/skilld-docker-container.git
2. cd skilld-docker-container
3. edit composer.json to remove patch #59
4. make all to spin up Drupal and everything it needs (requires docker-ce + docker-compose)
5. make behat (run multiple times until issue appears)

Running these behat tests sometimes creates deadlock on the sqlite database. It will not happen anymore if you re-add patch.

@Berdir, about #72 comment

According to #2966607-81: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock, the latest patches over there not just no longer blocked on this, they actually seem to *fix* this problem as well.

Whoever relies on this, can you test the latest patch from there and confirm that?

With given steps to reproduce, you will be running a 8.8 core Drupal (so including patch from the issue you are mentioning, isn't it?) and I can confirm that the issue is still reproduced with 8.8 and without patch #57.

Hope it helps!

The only problem with the patch is performance degradation - like twice longer!

testing system running now longer then 110 minutes https://dispatcher.drupalci.org/job/drupal_patches/25329/console
data to compare 50mins of clean core on sqlite https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/10987/

Alternative solution:
We can also solve the problem by making the solution optional. In the settings.php file we have the database settings. We could for SQLite add an extra key named "immediate_transactions", which defaults to false. If it is being set to true then we use the new PDOConnection class and in all other cases we would use the default PDO connection. We would need to add some documentation to the settings.php file and add a CR. Document the advantages and disadvantages of the alternative PDOConnection class.

#79 seems like a good approach to me.

With the approval of @catch, back to needs work.

This doesn't add any of the documentation, because this database stuff is frankly out of my pay grade. Nonetheless, this implements the optional solution described in #79.

> The only problem with the patch is performance degradation - like twice longer!

I read the whole issue and deep down in #32/#33 there may be an alternative:

i only see 2 sinple ways to approach this.
the first, always setting exclusive locks, sounds like a performance nightmare.
the other: if sqlite throws "busy", sleep and retry.

If it was just COMMIT that threw the SQLITE_BUSY that would be easy enough to adapt to, but it can be any read, write or transaction command. Exclusive locks would make SQLite unusable in anything other than testing, and even then requests for generated resources e.g. css could cause problems.

The only solution I can see is to wrap both PDO and PDOStatement in proxy classes, magic method most of the stuff through to the underlying objects and try/catch/sleep the PDO exec, query, beginTransaction and commit methods and the PDOStatement::execute() method. I did something similar for a Logging PDO class recently and it worked well...

And, documenting the back and forth in the IS we must...

Core SQLite tests do sometimes fail due to deadlocks - see https://www.drupal.org/pift-ci-job/2349364 for example.

Copying the test failure output here for when the job info is lost...

Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-110.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.19 �[44m#StandWith�[0m�[43mUkraine�[0m

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest
.E............                                                    14 / 14 (100%)

Time: 02:53.504, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testErrorMessages
Drupal\Core\Entity\EntityStorageException: SQLSTATE[HY000]: General error: 5 database is locked: UPDATE "test90328983"."node" SET "vid"=:db_update_placeholder_0, "type"=:db_update_placeholder_1, "uuid"=:db_update_placeholder_2, "langcode"=:db_update_placeholder_3
WHERE "nid" = :db_condition_placeholder_0; Array
(
    [:db_update_placeholder_0] => 1
    [:db_update_placeholder_1] => blog
    [:db_update_placeholder_2] => fa420a61-a2e9-459e-b0c1-17b48b2b0343
    [:db_update_placeholder_3] => en
    [:db_condition_placeholder_0] => 1
)


/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:811
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:230
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

#83:

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -118,8 +118,13 @@ public static function open(array &$connection_options = []) {
+    $connection_class = $connection_options['immediate_transactions']) ? __NAMESPACE__ . '\PDOConnection' : '\PDO';

This line has a closing brace ")" too much.

Apart from that, LGTM.

Did a reroll for 9.4 and removed the extra closing brace as per #90.

Interesting sqlite fails:
- Installer(!): Form field with id|name|label|value "sqlite[immediate_transactions]" not found.
- Media: PDOException: SQLSTATE[HY000]: General error: 5 database is locked

Regarding performance, i can't see the difference mentioned in #78 anymore, but maybe i'm missing something.
- mysql: 60 minutes total from scheduled to completion.
- sqlite: 50 minutes total from scheduled to completion.

EDIT: Probably that is expected, as the new option is added, but not enabled by default.

Can we try a patch that opts us into the fix (at least for test runs) to see if it will help with the situation in HEAD? Via @hestenet.

#92 is an example of how SQLite randomly breaks on HEAD test runs since the most recent DrupalCI update about a month ago. See https://www.drupal.org/pift-ci-job/2353435 for example. It would be interesting to see if this might mitigate that. If this would fix HEAD, I'd upgrade it to critical. Or, if HEAD still fails, we can at least rule this out as the cause of SQLite dying in 50% or more of HEAD test runs.

THx @xjm for clarifying, i did not have an eye on that.

Yes then i think this should be critical.

AND i think this approach does not go far enough.
Read the sqlite docs on https://sqlite.org/lang_transaction.html and https://sqlite.org/rescode.html#busy

An SQLITE_BUSY error can occur at any point in a transaction: when the transaction is first started, during any write or update operations, or when the transaction commits. To avoid encountering SQLITE_BUSY errors in the middle of a transaction, the application can use BEGIN IMMEDIATE instead of just BEGIN to start a transaction. The BEGIN IMMEDIATE command might itself return SQLITE_BUSY, but if it succeeds, then SQLite guarantees that no subsequent operations on the same database through the next COMMIT will return SQLITE_BUSY.

We get those busy errors, maybe from cron runs in another process, maybe from whatever.

We can implement a "retry" loop, but it seems that sqlite has it builtin, so we should set it on connection init.
https://sqlite.org/pragma.html#pragma_busy_timeout

trying @geek-merlin suggestion on connection busy timeout ...

Patch #59 needs re-roll

Just a reroll for #59

Patch applies on core without error but after we have this error on site install

You are about to:
 * CREATE the './../.cache/db.sqlite' database.

 // Do you want to continue?: yes.                                              

 [notice] Starting Drupal installation. This takes a while.
 [notice] Performed install task: install_select_language
 [notice] Performed install task: install_select_profile
 [notice] Performed install task: install_load_profile
 [notice] Performed install task: install_verify_requirements
Error: Class "Drupal\sqlite\Driver\Database\sqlite\PDOConnection" not found in Drupal\sqlite\Driver\Database\sqlite\Connection::open() (line 119 of /var/www/html/web/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php).

The new PDOConnection class needs to be moved to the sqlite module.

fixed patch with proper place and namespace

Fix return types to conform PHP 8.1

Promoting to critical as per my previous comment.

I've been using the patch in 108 and have run large incoming migrations, editing and parallel PHPUnit tests doing crud things simultaneously with this patch.

Previously the issue would occur regularly enough that I considered switching back to MySQL, now SQLite works perfectly fine and faster than MySQL as a bonus.

Solved the issue for me.

#108 took 1hr 1min on sqlite and the latest run on 9.5.x with the same environment to 47 min. That looks to be quite a big regression. Will re-run tests against #108 and see if the regression is still there.

So on PHP 7.3 performance is an issue - the test run aborted because of the time - see https://dispatcher.drupalci.org/job/drupal_patches/142869/consoleFull also the sqlite builds on 7.4 and 8.1 were slower so this change does represent a performance regression.

Swapping the random fails for DrupalCI timeouts does not seem like a good trade.

As of performance regression: There's no such thing as free locking, and some percent performance regression is to be expected as price for data integrity. So i suppose we must buy it and pay the price on PHP 7.4.
As of PHP 7.3, no need to keep running that, since #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4.

Let's RTBC and get this in.

I will airlift in here and say that I found this issue after running into the locked database error when running tests for Simple OAuth locally, on SQLite. The test in question saves a user repeatedly in quick succession (to test side-effects of removing roles) and I only got it to work after adding a sleep() as a test, between saves.

The patch in #108 resolved this. I will +1 the push to RTBC, especially since this is a real head-scratcher if your test suite all of a sudden throws this.

So setting to RTBC again with the following reasoning:
- The patch provides locking ensuring database integrity.
- For PHP 7.4+, it comes with a performance penalty of order of magnitude 10%, which is an acceptable and to-be-accepted price for locking.
- For PHP 7.3, the patch has issue. But as PHP 7.3 is not officially supported anymore, we are happy to accept this price (which may or may not be researched in a followup issue), to fix the supported and 99% use case.

@alexpott Please correct if sth of this doubted.

Swapping the random fails for DrupalCI timeouts does not seem like a good trade.

I can live with the price that we need to pay for having locking. Only the remark from @alexpott about the timeouts has me a little bit worried. As far as I understand, doing this patch makes working with SQLite a lot more reliable. Which is for me in the end the most important.

+1 for RTBC from me.

We still run tests on PHP 7.3 see the 9.5 daily tests on https://www.drupal.org/node/3060/qa so either we'll need a decision to stop doing that - or we'll need to fix it.

I also think the issue summary needs updating with comments about the performance compromise if we choose to make it.

We still run tests on PHP 7.3 see the 9.5 daily tests on https://www.drupal.org/node/3060/qa so either we'll need a decision to stop doing that - or we'll need to fix it.

A possible option would be to just not backport this to 9.5.x. Definitely agreed on documenting the performance regression, presumably will affect real sites too.

A possible option would be to just not backport this to 9.5.x.

+1 for this idea!

> A possible option would be to just not backport this to 9.5.x.

+1 too. Who wants this for D9 can composer-patch it in.

So this may be a way to get this in, once someone(tm) texts a suitable Change Record.

Took a first stab at a change record. https://www.drupal.org/node/3305282

Pulled the latest changes for 9.5.x today and started getting this error running any test.
Patch #108 solved this problem for me.

Hm. If this fixes the SQLite database locks, ideally we would want any fix backported to 9.5, 9.4, and even 9.3, because the database locks are making the SQLite test environments next to useless on those branches. So that would mean supporting PHP 7.3. I'd be okay committing the current approach to 10.0 and 10.1 and then working on a backport, although it sounds like the performance impact still needs review also. (Worst case, we could commit it and revert it.)

Is there much concurrency on the same sqlite db during DrupalCI execution? I know we run concurrent tests, but doesn't each test create its own database? In the absence of concurrency on the same db, I'm surprised the performance hit for simply acquiring the lock earlier in the transaction rather than later in the transaction is so large.

https://phiresky.github.io/blog/2020/sqlite-performance-tuning/ lists some performance tuning options. In particular, I wonder if pragma synchronous = normal; would help mitigate the performance cost of acquiring the lock earlier.

From #96:

The BEGIN IMMEDIATE command might itself return SQLITE_BUSY

Wouldn't that be a problem given Drupal's calling code? Typically, we call $this->database->startTransaction() outside of a try/catch. So by moving from DEFERRED to IMMEDIATE, we're moving from an exception being thrown where it can be caught, to an error state (exception?) that isn't caught. However, #110 seems to indicate that such an error doesn't happen in practice, at least in @singularo's testing. Why is that?

As this issue is now a critical one and has the attention of 2 core committers, hopefully it will now get fixed.
One of the main problems with testing is that we have a high number of queries on the database and for SQLite that is a problem. One thing that we can do is trying to lower the number of queries. The current patch of #2031261: Make SQLite faster by combining multiple inserts and updates in a single query is doing that. It does that by merging multiple inserts into a single table into a single insert. The same for multiple upsert queries into the same table. It also makes the testbot for SQLite finish much quicker. I am very curious to see, if with that fix the problems of this issue also be fixed.
To the core committers: If we are going to do that, can the other issue than also be critical?

I opened #3313142: Test issue for SQLite immediate/deferred transactions as a child issue to try to identify which transactions cause a slow down when changed to IMMEDIATE. So far, I discovered that the most important one (SqlContentEntityStorage::save()) does not cause a slow down of DrupalCI tests, at least not on its own (#5 in that issue took the same amount of time as #2).

I intend to throw more patches at that issue this week to figure it out.

What I learned in #3313142: Test issue for SQLite immediate/deferred transactions is that most of the performance cost to running core's tests on DrupalCI of changing to IMMEDIATE transactions is when executing a single INSERT statement within a transaction. Which there's no reason to do anyway, so this takes #108, but also changes Drupal\sqlite\Driver\Database\sqlite\Insert to only use a transaction for a multi-insert when it is truly a multi-insert (not a multi of 1).

This brings the extra time that it takes to run tests on DrupalCI to +5% for PHP 7.4 or higher and +20% for PHP 7.3. One difference is that PHP 7.4 and higher use the SQLite that's on the operating system while PHP 7.3 has SQLite compiled into PHP, so that might explain why the impact is so different between the versions, though I don't know why exactly PHP 7.3's internal SQLite would be more sensitive to the change, and perhaps that's a coincidence and there's a different reason for the difference. However, while we still technically support PHP 7.3 for Drupal 9, I think we care less and less about it, so the +5% impact is probably the more important one to us than the +20% one.

The next two places that account for most of that +5% are MenuRouterRebuildSubscriber::menuLinksRebuild() and Drupal\Core\Routing\MatcherDumper::dump(). If we could keep those two places using DEFERRED, we could claw back most of that 5%, and I experimented with doing that in #3313142: Test issue for SQLite immediate/deferred transactions. However, I'm not sure it's worth doing that, so I left it out of this patch. Because to do so would require creating an API through which to do it (e.g., by adding another parameter to startTransaction() which is what I did in the test patches in that issue), but if this is a SQLite-specific concept, then it's kind of messy to leak that into APIs that should be database agnostic. In other words, I think it would be better for us to just accept the 5% penalty to DrupalCI runs, or if we don't want to accept that, then to open a followup for discussing how to create the API for callers to specify when to use DEFERRED.

With regards to #125, this patch moves all of the startTransaction() calls into the try block in the places where the catch block does something other than just roll back the transaction. This way, this change doesn't introduce a subtle BC break of failing to log what used to be logged or throwing different exceptions than what existing callers might be expecting.

I also modified the PHPDoc for PDOConnection from what was in #108. Please review the updated wording.

Re #126, I think this can proceed without #2031261: Make SQLite faster by combining multiple inserts and updates in a single query, though thank you for reminding me about that issue.

With this patch, I don't have any remaining concerns about this getting committed to 9.5 and higher. For committing to 9.4 and 9.3, I'm not sure how much we should care about the potential performance impact this might have to existing sites. The numbers we're seeing from DrupalCI might not reflect the same kind of traffic pattern that real sites get, so I don't know what the impact to real sites would be. But also, running SQLite on production for a high concurrency application seems pretty edge case to me. If we think it's appropriate to do so, we could add a settings.php setting for existing sites to turn this off?

Added the standard template to the IS and items from #123 and #128. I did not read the entire issue, so leaving the 'needs issue summary update' tag.

I'll update the issue summary tomorrow, but I'm not assigning the issue to myself, since the patch can be reviewed in the meantime.

The build times (not counting waiting in the queue times) for the tests in #128 are surprisingly fast. Compared to the most recent HEAD runs, I'm seeing:

To me, it seems more likely that these were some outlier builds rather than indicative that #128 improves performance, so it would be worth repeating the test runs, perhaps at a time when we're not committing other patches so we don't have a shifting baseline.

Instead of re-queueing tests for #128 (which might remove the existing build artifacts that might be useful for analyzing if indeed those DrupalCI runs were outliers), the "full" patch in this comment is identical to the one in #128.

This also includes a no-op patch, as a way to test performance of HEAD without losing the build artifacts when HEAD itself gets re-tested.

And this also includes a "partial" patch which contains all of the changes in "full" except the most important part (the change from DEFERRED to IMMEDIATE), so as to identify the performance impact of all of those other changes separately from the DEFERRED to IMMEDIATE change.

The build times in #132 match the 20% for PHP 7.3 and 5% for PHP 7.4+ that I claimed in #128. Here's the table:

Note that the runs in #132 don't all pass due to random failures, and a test that fails can take either less time (if it fails early) or more time (if the failure is due to waiting on something that times out before returning) than a test that passes. However, since it's only the difference of one or two tests, I don't know if that impacts the total DrupalCI time by more than a minute. Ideally, we could get a batch of runs that all run against the same state of HEAD and where none encounters a random failure, but that might take several attempts to achieve. In the meantime, I think the above table is roughly accurate.

+++ b/core/modules/sqlite/src/Driver/Database/sqlite/PDOConnection.php
@@ -0,0 +1,53 @@
+    return $this->exec('BEGIN IMMEDIATE TRANSACTION') !== FALSE;

I still need to carve out a bit of time to update the issue summary, but in the meantime, I wanted to share the question I asked in the SQLite forum: https://sqlite.org/forum/forumpost/07cc5acfcc01b7f6. The answers there are further confirmation that this part of the patch is both necessary and correct.

Maybe it should try start deffered transaction eith shorter timeout and if it fail to attempt immediate one?

The challenge with #135 would be that with deferred transactions, the "database is locked" exception is triggered during the first write statement in the transaction (after 1 or more read statements have been executed), and when that occurs depends on the calling code. E.g., in a different place for SqlContentEntityStorage::save() than for MenuRouterRebuildSubscriber::menuLinksRebuild(), etc. So at present, it would need to be that calling code that catches the exception, rolls back the transaction, and retries with BEGIN IMMEDIATE. But that then means leaking the entire concept of DEFERRED vs IMMEDIATE (which is a SQLite-specific concept) to callers that ideally shouldn't have to know about database-driver-specific concepts.

An interesting abstraction would be if all callers of ->startTransaction() were refactored into closures, and then for the database API to implement generic support for retrying transactions by re-invoking that closure. That seems out of scope for this issue, but might be a cool followup.

#135 / #136 might not be that big a change to pull off. For example, MenuRouterRebuildSubscriber::menuLinksRebuild() currently does:

try {
  $transaction = $this->connection->startTransaction();
  $this->menuLinkManager->rebuild();
  $this->replicaKillSwitch->trigger();
}
catch (\Exception $e) {
  if (isset($transaction)) {
    $transaction->rollBack();
  }
  watchdog_exception('menu', $e);
}

In theory, that could be refactored to something like:

try {
  $this->connection->executeRetryableTransaction(function () {
    $this->menuLinkManager->rebuild();
    $this->replicaKillSwitch->trigger();
  }, 2);
}
catch (\Exception $e) {
  watchdog_exception('menu', $e);
}

Where "2" in the above code is the number of times to attempt. The executeRetryableTransaction() method could then take on the responsibility for rolling back, removing a little boilerplate from the caller, and the SQLite driver could then have code that for retryable transactions it does the first attempt with BEGIN DEFERRED and the 2nd through Nth attempts as BEGIN IMMEDIATE.

But to do this, we'd need to add the executeRetryableTransaction() method to the database API and work out whatever challenges arise in implementing that, so I think that's still better left to a followup, but at least at first glance it seems potentially doable.

An interesting abstraction would be if all callers of ->startTransaction() were refactored into closures, and then for the database API to implement generic support for retrying transactions by re-invoking that closure. That seems out of scope for this issue, but might be a cool followup.

This is a neat idea; closures are something I think we could use more of in core (another example: cache gets, where the fallback to calculate an uncached value lives in a closure) and something we could explore now in a separate issue.

Updated the issue summary.

The story from @effulgentsia makes it clear to me that the changed proposed by him is a good one. Including his discussion on the SQLite forum.

1. I think we should update the examples with the use of transactions in core/lib/Drupal/Core/Database/database.api.php. We are doing to do it differently and the examples should reflect that.
2. In core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php we have on the lines 769, 1730 and 1821 we have a transaction that is started out side of the try-catch block. Can we move it into the try-catch block?
3. The same for the core/lib/Drupal/Core/Menu/MenuTreeStorage.php on line 313.
4. Also for core/modules/sqlite/src/Driver/Database/sqlite/Connection.php on line 402.

5. +++ b/core/modules/sqlite/src/Driver/Database/sqlite/Insert.php
   @@ -35,41 +39,60 @@ public function execute() {
   +      if (count($this->insertValues) === 1) {
   +        // A single row can be inserted without the overhead of a transaction.
   

   I am not sure why a single insertion can go without a transaction?

6. +++ b/core/modules/sqlite/src/Driver/Database/sqlite/Insert.php
   @@ -35,41 +39,60 @@ public function execute() {
   +          throw new DatabaseExceptionWrapper($e->getMessage(), 0, $e);
   

   Why not use $this->connection->exceptionHandler()->handleExecutionException() here?

Re-roll of #132 to keep things truck'n along.

My review points from #140 still need to be addressed.

Thanks for the review in #140. This addresses all those points.

For #2 and #3, moving ->startTransaction() into the try statement isn't strictly needed if all that the catch block does is roll back and rethrow the same exception, without logging or wrapping the exception or anything else, since there's nothing to roll back if we didn't successfully start the transaction to begin with. However, I agree with your point in #1 about the docs, and since this patch updates those docs, then yeah, I think there's a consistency benefit to always putting the ->startTransaction() into the try statement, even if there's no functional difference to doing so.

+++ b/core/lib/Drupal/Core/Database/Query/Insert.php
@@ -79,22 +79,24 @@ public function execute() {
+      // Per https://en.wikipedia.org/wiki/Insert_%28SQL%29#Multirow_inserts,

The testbot is complaining that the word "multirow" is an unknown word.

I will do a full review later.

@effulgentsia: Thank you for working on this!

This fixes #146 and also a copy/paste error.

All code changes look good to me.
All my points have been addressed.
For me it is RTBC.

+++ b/core/modules/sqlite/src/Driver/Database/sqlite/Insert.php
@@ -35,41 +39,64 @@ public function execute() {
+            // One of the INSERTs failed, rollback the whole batch.

Shouldn't this also have a break; - we don't want to keep trying to insert rows in the rest of the foreach after rolling back the transaction. Or if that's not necessary for some reason it would be good to document it.

We do not need to add a break;, because 2 lines later the following code is run: $this->connection->exceptionHandler()->handleExecutionException($e, $stmt, $insert_values, $this->queryOptions); which will result in an exception being thrown.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x.

I think we should backport this to 9.4.x too but let's get clean test runs on HEAD at least before doing so (if the js failures are cleared up enough to do so). Could also choose to backport just after the next patch release.

Did my best with issue credits but this is a very long issue with a lot of people on it so apologies if anything's overlooked.

Do you want to wait with publishing the CR until it's backported to 9.4?

I made some edits to the original CR and added a 2nd CR for moving ->startTransaction() into the try block. Left both unpublished pending review and deciding whether or not to wait for the 9.4 backport per #153.

The 2nd CR looks good to me.

In the first the Drupal version is changed to 9.5. Why?

#151 committed this to 9.5.

@effulgrentsia: You are right, about it being D9.5

I have published the both CR's as I am the subsystem maintainer of the Database API.

Sorry publishing the CRs is good, we can always move the version back after backport.

patch works fine on 9.4.x - is it going to be backported?

🎉Amazing work!

Reviewed as part of Bug Smash Initiative. It's unclear what the next steps are here. I think the remaining tasks in the issue summary should be updated to reflect that this has been committed to 9.5/10.0/10.1 and needs a backport to 9.4. Although #159 suggests this is possibly RTBC? Is that for the patch in #147?

Discussed this further in Slack. There is at least one green run for this for Drupal 9.4 / PHP 7.4 and the other fails seem to be due to flaky webdriver tests. I think #147 is RTBC for 9.4.x.

Thanks for the triage, agreed this is ready. Committed/pushed to 9.4.x, thanks!

Really great to see this fixed.