Problem/Motivation
SQLite does not support row level locks, so when it executes a write statement it locks the entire database. Because of this, SQLite tries to minimize the amount of time it holds a write lock. One way it does that is that by default, rather than reserving a write lock at the beginning of a transaction, it defers that until the first write statement of the transaction.
However, this leads to deadlocks that become increasingly likely the more time passes between a transaction's first read statement and its first write statement. In Drupal, we have some code paths where a non-trivial amount of PHP code is executed between such statements, including invoking hooks such as hook_entity_presave() and others.
As a result, numerous people over the course of this issue have encountered the deadlock, in the form of a "database is locked" error message, and DrupalCI occasionally fails on a random test with it.
Steps to reproduce
Proposed resolution
- Within the SQLite driver, override
\PDO::beginTransaction()
to begin transactions in IMMEDIATE mode, which reserves the write lock immediately and therefore prevents deadlock. - However, doing this decreases performance of transactions. Therefore, this patch also changes the SQLite driver's
Insert
class to not wrap INSERTs of a single row in a transaction, since that's unnecessary and likely only existed in a transaction wrapper in HEAD because SQLite's deferred transactions didn't impose a penalty for it. - Even with the above, there's still a net 5% increase for PHP 7.4+, and a net 20% increase for PHP 7.3, in the time it takes for DrupalCI to run all of core's tests on SQLite (see #133). Reducing this might be possible (see #128 and #135 - #138), but what's being currently proposed for this issue is to punt those ideas to followups, and accept the performance hit for now.
- With the change to IMMEDIATE mode, it is now possible for
->startTransaction()
to itself throw an exception due to not being able to acquire a write lock, if another process has a write lock for longer than the timeout. However, this is less likely than the current deadlock problem, since this is not a deadlock but just a normal condition of waiting for another process to finish an operation that it isn't blocked on finishing. Still, because an exception at this step becomes possible, this patch also moves the->startTransaction()
calls into thetry
block when thecatch
block does more than just roll back the transaction.
Remaining tasks
- Review the patch.
- Decide if the performance cost is acceptable enough to commit the patch.
- Decide if we want to add a setting to allow site owners to override the decision to use IMMEDIATE, and to stick with DEFERRED: basically if site owners would prefer the occasional deadlock over the performance cost.
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#147 | interdiff.txt | 1.35 KB | effulgentsia |
#147 | 1120020-147.patch | 23.92 KB | effulgentsia |
Issue fork drupal-1120020
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
rfayTrying sqlite 3.7 now with the PPA from https://launchpad.net/~aleksander-m/+archive/sqlite3
Comment #2
dpovshed CreditAttribution: dpovshed commentedHello 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
Comment #3
rfayThat's pretty scary. Are you able to recreate the "database is locked" issue with a particular sequence?
I've updated to sqlite 3.7.2 and am running my local dev Drupal Commerce install using it, and haven't seen this since I updated sqlite.
Comment #4
rfayI should mention that the great new project DBTNG Migrator allows you a migration path from sqlite, so there are now options for people who deploy on sqlite.
Comment #5
rfayGot this tonight just using my (freshly created Commerce Quickstart) database. I just opened one page while another was loading. This is Ubuntu 10.04 with Sqlite 3.7.2
Comment #6
dpovshed CreditAttribution: dpovshed commentedRandy,
answering to your comment #3.
With 95% chances I can reproduce error like
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.
Comment #7
rfayDamZ suggests that we set a "Busy Timeout": http://www.sqlite.org/c3ref/busy_timeout.html
Comment #8
rfay@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.
I tried the attached patch after a bit of investigation of how you do this through set the timeout using PDO. Setting CNR, but nothing the testbot does will actually test it, and this doesn't work as determined by manual testing.
It made the error be delayed for 60 seconds, but didn't help with the problem. I suspect that we actually have a locking problem of some type.
This seems not to be an unusual problem in PDO sqlite. Some references
We could also do exception catching on this error.... This comment in the PHP manual shows a very ugly approach to this.
Due to this issue I don't think we should be recommending sqlite3 as a solution for deploying Drupal at this time. Sad panda.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat'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.
Comment #10
bfroehle CreditAttribution: bfroehle commentedThe 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.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedGrepping the PHP source code, I don't think PHP is setting a default value for this.
Comment #12
bfroehle CreditAttribution: bfroehle commentedDamien: Well, the source code is always authoritative. :) My comment above was based on http://bugs.php.net/bug.php?id=38182
Comment #13
rfayPutting to its correct status, as it doesn't actually solve the problem.
@DamZ, the timeout would be expected, agreed: This is just a way to demonstrate the fatal so that (hopefully) we can build a patch to avoid it.
Comment #14
Garrett Albright CreditAttribution: Garrett Albright commentedEverything 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.
Comment #15
Garrett Albright CreditAttribution: Garrett Albright commented60 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.
Comment #16
Garrett Albright CreditAttribution: Garrett Albright commentedPossibly related email thread - from five years ago…
Comment #17
Garrett Albright CreditAttribution: Garrett Albright commentedWTF 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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented#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?
Comment #19
Garrett Albright CreditAttribution: Garrett Albright commentedOh, 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.
Comment #20
Garrett Albright CreditAttribution: Garrett Albright commentedTook 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:
This is a hellbug.
Comment #21
rfayI'm not comfortable with the renaming/explicit focus of this. It's been easy to demonstrate in other contexts than search index updates (I believe). Can we leave it as a generic title for now? If you're sure that it's really just the search stuff I'm ok with it.
Comment #22
dpovshed CreditAttribution: dpovshed commented@rfay: I think remaning is correct. Search is still the best option to reveal this bug, anybody can easily find this in thread text.
Comment #23
catchIf 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.
Comment #24
chx CreditAttribution: chx commentedJust a random idea, besides upgrading to 3.7 also try http://www.sqlite.org/draft/wal.html this.
Comment #25
dpovshed CreditAttribution: dpovshed commentedGood 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.
Comment #26
geek-merlinit 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.
Comment #27
chx CreditAttribution: chx commentedYes 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:
I fail to see how could we keep the statement alive for a shorter time. Does something keep the statement object alive??
Comment #28
geek-merlinthanks 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?):
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)
Comment #29
chx CreditAttribution: chx commentedOf 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.
Comment #30
tf198 CreditAttribution: tf198 commentedI'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:
but this will throw the error if the DB is locked by another process.
You can fix this by using an IMMEDIATE or EXCLUSIVE lock as opposed to the default DEFERRED so the following should work as expected:
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
Comment #31
tf198 CreditAttribution: tf198 commentedA 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()
and sometimes so does the other process as the lock is not correctly released
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.
Comment #32
geek-merlinthank 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:
Comment #33
tf198 CreditAttribution: tf198 commentedIf 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...
Comment #34
geek-merlinof 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.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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):
If you launch two of those, I see the expected sequence:
If I change the
BEGIN
into aBEGIN IMMEDIATE
, I also see the expected:Anyway, we DO want to switch to
BEGIN IMMEDIATE
in Drupal (it should be as easy as overridingPDO::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 supportSELECT FOR UPDATE
queries.Comment #36
geek-merlini 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.
Comment #37
tf198 CreditAttribution: tf198 commentedQuick 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.
Comment #38
geek-merlini 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.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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).Comment #40
geek-merlinhere is a patch for this to review.
also crosslinking #1764454: Erroneous static PDO calls in DB driver.
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks 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.Comment #42
Anthony Fok CreditAttribution: Anthony Fok commentedHello 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 BooleanFALSE
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: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 BooleanFALSE
is indeed raised, exactly matching the behaviour of the originalPDO::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!Drupal\Core\Database\Driver\sqlite\Connection->pushTransaction()
checks forsavepointSupport
, which isTRUE
for SQLite >= 3.6.8, and proceeds to callparent::pushTransaction()
, never calling the newbeginTransaction()
...PDO::beginTransaction()
... Oh, #1764454: Erroneous static PDO calls in DB driver is probably an attempt to fix this.So, I am stuck. Oops...
Comment #43
steinmb CreditAttribution: steinmb commentedTagging
Comment #44
heddnTriaging 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.
Comment #45
cyborg_572 CreditAttribution: cyborg_572 at Northern Commerce commentedSprinting at Drupal North, I'm going to attempt to re-roll this.
Comment #46
cyborg_572 CreditAttribution: cyborg_572 at Northern Commerce commentedAttaching re-rolled patch.
Comment #47
daffie CreditAttribution: daffie commentedThe 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.
Comment #48
amateescu CreditAttribution: amateescu as a volunteer commentedThe 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 overridecommit()
androllBack()
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.
Comment #49
MixologicPatch testing on sqlite works on drupalci. I would resubmit the patch to be able to do that.
Comment #50
amateescu CreditAttribution: amateescu for Drupal Association, Pfizer, Inc. commentedWe 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.
Comment #52
daffie CreditAttribution: daffie commentedLets see how much slower the testbot get with this patch. The patch is a reroll of the patch from comment #40.
Comment #53
daffie CreditAttribution: daffie commentedI think that the locking problems with SQLite will be gone if we fix #2348137: Enable WAL journal mode by default for SQLite database.
Comment #54
andypostAs #25 said wal does not solve the issue
Comment #58
pfrenssenStraight reroll to 8.4.x.
Comment #59
pfrenssenBrought the patch in line with our current standards by removing the file docblock.
Comment #60
pfrenssenWe 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.
Comment #62
pfrenssenJust verified it, the patch still applies correctly to 8.5.x.
Comment #63
daffie CreditAttribution: daffie commentedComment #64
daffie CreditAttribution: daffie commentedUpdated the issue summary.
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedPHP 7 & SQLite 3.8 CI aborted
due to problem in theQuickStartTest
. For some reason this process hangs on DI. See #2975644-12: Random Failure in Drupal\Tests\Core\Command\QuickStartTest.Also maybe this issue can help to #2966607-36: Invalidating 'node_list' and other broad cache tags early in a transaction severely increases lock wait time and probability of deadlock. But so far with the #59 patch this has not happened.
Comment #66
idebr CreditAttribution: idebr at iO commentedThe 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
Comment #67
steinmb CreditAttribution: steinmb as a volunteer commented@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
Comment #68
Wim LeersThis 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.Comment #70
andypostIs this really hard blocker?
Comment #71
pcambra[#59] solves the issues I've been having locally.
Comment #72
BerdirAccording 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.
Comment #73
pcambraThanks 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.
Comment #75
toamit CreditAttribution: toamit commented@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).
Comment #77
davidferlay CreditAttribution: davidferlay at Skilld commentedHi 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 :
git clone https://github.com/skilld-labs/skilld-docker-container.git
cd skilld-docker-container
make all
to spin up Drupal and everything it needs (requires docker-ce + docker-compose)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
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!
Comment #78
andypostThe 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/
Comment #79
daffie CreditAttribution: daffie commentedAlternative 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.
Comment #80
catch#79 seems like a good approach to me.
Comment #81
daffie CreditAttribution: daffie commentedWith the approval of @catch, back to needs work.
Comment #83
phenaproximaThis 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.
Comment #84
geek-merlin> 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:
And, documenting the back and forth in the IS we must...
Comment #88
alexpottCore SQLite tests do sometimes fail due to deadlocks - see https://www.drupal.org/pift-ci-job/2349364 for example.
Comment #89
alexpottCopying the test failure output here for when the job info is lost...
Comment #90
geek-merlin#83:
This line has a closing brace ")" too much.
Apart from that, LGTM.
Comment #91
andregp CreditAttribution: andregp at CI&T commentedDid a reroll for 9.4 and removed the extra closing brace as per #90.
Comment #92
geek-merlinInteresting 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
Comment #93
geek-merlinRegarding 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.
Comment #95
xjmCan 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.
Comment #96
geek-merlinTHx @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
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
Comment #99
Taran2Ltrying @geek-merlin suggestion on connection busy timeout ...
Comment #100
andypostComment #102
andypostPatch #59 needs re-roll
Comment #103
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedJust a reroll for #59
Comment #104
3_k_3_n CreditAttribution: 3_k_3_n at Skilld commentedPatch applies on core without error but after we have this error on site install
Comment #105
daffie CreditAttribution: daffie commentedThe new PDOConnection class needs to be moved to the sqlite module.
Comment #106
andypostfixed patch with proper place and namespace
Comment #108
andypostFix return types to conform PHP 8.1
Comment #109
xjmPromoting to critical as per my previous comment.
Comment #110
singularoI'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.
Comment #111
alexpott#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.
Comment #112
alexpottSo 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.
Comment #113
geek-merlinAs 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.
Comment #114
bradjones1I 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.
Comment #115
geek-merlinSo 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.
Comment #116
daffie CreditAttribution: daffie commentedI 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.
Comment #117
alexpottWe 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.
Comment #118
catchA 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.
Comment #119
daffie CreditAttribution: daffie commented+1 for this idea!
Comment #120
geek-merlin> 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.
Comment #121
bradjones1Took a first stab at a change record. https://www.drupal.org/node/3305282
Comment #122
smustgrave CreditAttribution: smustgrave at Mobomo commentedPulled the latest changes for 9.5.x today and started getting this error running any test.
Patch #108 solved this problem for me.
Comment #123
xjmHm. 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.)
Comment #124
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs 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.Comment #125
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom #96:
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?Comment #126
daffie CreditAttribution: daffie at Finalist commentedAs 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?
Comment #127
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #128
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat 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()
andDrupal\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 thecatch
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?
Comment #129
quietone CreditAttribution: quietone at PreviousNext commentedAdded 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.
Comment #130
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'll update the issue summary tomorrow, but I'm not assigning the issue to myself, since the patch can be reviewed in the meantime.
Comment #131
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe 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.
Comment #132
effulgentsia CreditAttribution: effulgentsia at Acquia commentedInstead 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.
Comment #133
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe 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.
Comment #134
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #135
andypostMaybe it should try start deffered transaction eith shorter timeout and if it fail to attempt immediate one?
Comment #136
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe 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 forMenuRouterRebuildSubscriber::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.Comment #137
effulgentsia CreditAttribution: effulgentsia at Acquia commented#135 / #136 might not be that big a change to pull off. For example,
MenuRouterRebuildSubscriber::menuLinksRebuild()
currently does:In theory, that could be refactored to something like:
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.
Comment #138
longwaveThis 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.
Comment #139
effulgentsia CreditAttribution: effulgentsia at Acquia commentedUpdated the issue summary.
Comment #140
daffie CreditAttribution: daffie at Finalist commentedThe 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.
I am not sure why a single insertion can go without a transaction?
Why not use
$this->connection->exceptionHandler()->handleExecutionException()
here?Comment #142
kim.pepperRe-roll of #132 to keep things truck'n along.
Comment #144
daffie CreditAttribution: daffie at Finalist commentedMy review points from #140 still need to be addressed.
Comment #145
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks 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.
Comment #146
daffie CreditAttribution: daffie at Finalist commentedThe 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!
Comment #147
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis fixes #146 and also a copy/paste error.
Comment #148
daffie CreditAttribution: daffie at Finalist commentedAll code changes look good to me.
All my points have been addressed.
For me it is RTBC.
Comment #149
catchShouldn'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.Comment #150
daffie CreditAttribution: daffie at Finalist commentedWe 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.Comment #152
catchCommitted/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.
Comment #153
Wim LeersDo you want to wait with publishing the CR until it's backported to 9.4?
Comment #154
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #155
daffie CreditAttribution: daffie at Finalist commentedThe 2nd CR looks good to me.
In the first the Drupal version is changed to 9.5. Why?
Comment #156
effulgentsia CreditAttribution: effulgentsia at Acquia commented#151 committed this to 9.5.
Comment #157
daffie CreditAttribution: daffie at Finalist commented@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.
Comment #158
catchSorry publishing the CRs is good, we can always move the version back after backport.
Comment #159
andypostpatch works fine on 9.4.x - is it going to be backported?
Comment #160
geek-merlin🎉Amazing work!
Comment #161
mstrelan CreditAttribution: mstrelan at PreviousNext commentedReviewed 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?
Comment #162
mstrelan CreditAttribution: mstrelan at PreviousNext commentedDiscussed 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.
Comment #164
catchThanks for the triage, agreed this is ready. Committed/pushed to 9.4.x, thanks!
Comment #165
xjmReally great to see this fixed.