Problem/Motivation
By default, Drupal uses the REPEATABLE READ
transaction isolation level - this results in lots of deadlock errors.
Drupal.org and many other high traffic sites use READ COMMITTED
, so default to that instead. Drupal commerce has been recommended READ COMMITTED
since before 2014.
Steps to reproduce
In some circumstances, drush cr + loading a single page in a browser can be enough to trigger a deadlock.
Proposed resolution
Do 2 things:
- Add the possibility for siteowners to set the transaction isolation level in the database settings array in
setttings.php
; - Change the default transaction isolation level for new sites from
REPEATABLE READ
toREAD COMMITTED
.
Remaining tasks
TBD
User interface changes
In the database configuration for MySQL, MariaDB or equivalent databases in the installation proces can now the transaction isolation level be set.
API changes
The transaction isolation level can be set in the database settings array in setttings.php
Data model changes
None
Release notes snippet
Drupal now sets the READ COMMITTED transaction isolation level by default for new installs on MySQL. This level has already been recommended for several years and is configurable as before in the database connection settings.
Comment | File | Size | Author |
---|---|---|---|
#248 | interdiff.1650930.243-248.txt | 10.63 KB | longwave |
#248 | 1650930-248.patch | 17.99 KB | longwave |
| |||
#243 | interdiff-241-243.txt | 690 bytes | gidarai |
#243 | 1650930-243.patch | 18.85 KB | gidarai |
#239 | 1650930-239.patch | 18.9 KB | gidarai |
Issue fork drupal-1650930
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
deviantintegral CreditAttribution: deviantintegral commentedComment #2
tim.plunkettIsn't this rather edge-case-y for a major? Apologies in advance if I'm wrong.
Otherwise, this is in line with other uses of db_transaction().
CNW for tests.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt should not. We start a transaction in
MergeQuery::execute()
. Not sure what you are describing, but it's definitely not the expected behavior.Do you have a way to reliably reproduce the issue?
Comment #4
deviantintegral CreditAttribution: deviantintegral commentedI've got some code that generates nodes but it's currently tied to OG and devel. It looks like the rollback is actually happening in the cache_set() call in field_attach_load(), and not in node_save. But you're right, reading the code it does look like only the merge on the cache table should be rolled back and not the whole node object. I'll see if I can get this down to a test case and find what exception is causing the rollback.
Comment #5
deviantintegral CreditAttribution: deviantintegral commentedSo further investigation lead me to the the real source of the exception, which was a deadlock on the cache table. By default, MySQL is using next-row locking caused by the FOR UPDATE cause in the MergeQuery execute method. By settings the isolation level to READ COMMITTED, we only lock the single row. This can apparently cause problems if you are using replication and are *not* using row locking, but my understanding is that using row locking is pretty much required for Drupal installations.
The attached patch only sets the isolation level for MySQL as READ COMMITTED is the default in PostgreSQL according to http://www.postgresql.org/docs/9.1/static/sql-set-transaction.html.
Haven't tested this yet, and still need a test case.
Comment #6
chx CreditAttribution: chx commentedWell, if you have MySQL 5.1 then sure http://harrison-fisk.blogspot.ca/2009/02/my-favorite-new-feature-of-mysq... you want READ COMMITTED. But we require http://drupal.org/requirements/ only 5.0.15. Is http://ronaldbradford.com/blog/understanding-mysql-innodb-transaction-is... this situation a problem?
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is pretty scary: http://bugs.mysql.com/bug.php?id=45357
Comment #8
stewartsmith CreditAttribution: stewartsmith commentedI wouldn't go READ_COMMITTED for InnoDB, the default (REPEATABLE READ) is WAY more tested. That being said, if you just want READ COMMITTED for a particular operation, just using it for that operation should be fine.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedThings have evolved since last year. It is now clear that running a reasonably busy Drupal 7 website under
REPEATABLE READ
is just suicide. Drupal.org (and several other busy sites that I know of) are successfully running onREAD COMMITTED
.I'm marking #2164849: Enforce READ COMMITTED transaction isolation level as a duplicate, here is what I said there:
Comment #10
mikeytown2 CreditAttribution: mikeytown2 commentedRecently switched to READ COMMITTED as well and things are looking a lot better on our setup.
From my last comment on that dup thread:
Thinking we should set this as the default for all new Drupal installs on MySQL.
Do we want to put the code in the install form (install_settings_form) as a hidden field or drop it in at a later stage (install_settings_form_submit)?
Comment #11
andypostthis should use injected
$this->connection
Comment #12
vinmassaro CreditAttribution: vinmassaro commentedWe tested READ-COMMITTED level and it eliminated deadlocks but we noticed when using replication, it's possible that replication falls behind and gets out of sync without being able to catch up. Has anyone else run into this? I'm not sure if this situation has been considered based on wanting to make this a default in #10. Thanks.
Comment #13
mikeytown2 CreditAttribution: mikeytown2 commentedWe have 1 slave for replication and have not see any issues with it falling behind so far.
Comment #14
vinmassaro CreditAttribution: vinmassaro commentedIn a replicated MySQL environment, using READ-COMMITTED isolation requires using row-based replication and it's critical to have PRIMARY or UNIQUE keys in all your tables that you change. When you have InnoDB tables without primary keys, and use ROW based replication then you'll hit this bug: http://bugs.mysql.com/bug.php?id=53375. We observed large replication delays and it was just when load testing a single database that was set to READ-COMMITTED (one of many on the database server). We found some tables without primary keys:
Comment #15
mikeytown2 CreditAttribution: mikeytown2 commentedQuery to find tables without a primary or unique key from the comments of this article http://datacharmer.blogspot.com/2011/09/finding-tables-without-primary-k...
Tables that have issues on my site:
taxonomy_index is in core so that should be fixed. Running this query on this table shows the duplicate entries
On my database, by looking at the created column it appears that this is an error as the created column has the same date across all rows where the nid and tid are the same.
In this case for the core table using ALTER IGNORE TABLE seems like the best way forward.
Comment #16
mikeytown2 CreditAttribution: mikeytown2 commentedAlso noted that I keep getting deadlocks on the cache_field when cid = field_info_types:en. I created an issue for this #2193149: Deadlocks occur in cache_field table..
Also noted that the semaphore table has issues as well. Using a MEMORY table and everything seems good once I switched to BTREE #1898204-17: Do not use InnoDB for the semaphore table, use Memory
Comment #17
mikeytown2 CreditAttribution: mikeytown2 commentedWith the 2 changes in the above issue, it has been 24 hours since the last deadlock according to
SHOW ENGINE innodb STATUS
which is a first for us in a long time.Comment #18
andypostSo what is a proposed solution here?
Seems better to add this to default database settings line #10 suggests
Comment #19
mikeytown2 CreditAttribution: mikeytown2 commentedI would like to go forward with #10; if no one has any suggestions on where to put this I'll put in in the advanced options section inside DatabaseTasks::getFormOptions
Also noted #2222635: Waiting for table metadata lock on cache_field table. With this issue I'm opting for table rename without the drop.
Comment #20
mikeytown2 CreditAttribution: mikeytown2 commentedPatch needs a little bit of cleanup and we might only want to show that field when the database is of the type mysql.
Comment #22
mikeytown2 CreditAttribution: mikeytown2 commentedMade patch against 7.x; will flip back to 8.x once the test passes.
Comment #23
mikeytown2 CreditAttribution: mikeytown2 commented20: drupal-1650930-20-read-committed.patch queued for re-testing.
Comment #24
mikeytown2 CreditAttribution: mikeytown2 commentedComment #25
danblack CreditAttribution: danblack commentedwhile adding an initial connection string as a configuration option may seem reasonable I've got two reservations
a) The mysql default of REPEATABLE-READ makes more sense, especially in the merge function, so i'd suggest a blank value here as default.
b) as a global setting you don't know what assumption other modules/core bits have assumed (but they shouldn't assume),
b) Isn't setting this on a DB global place like my.cnf more appropriate (sure its not accessible if you don't manage the server).
If you want to select a transaction isolation level it probably should be specified in the db_transaction arguments (it won't nest however).
My gut feel is the db_merge function should be done at SERIALIZABLE isolation level to prevent updates between the select and insert implementation in merge. (and this function should be implemented as a INSERT OR REPLACE for mysql/sqlite where keys are primary or unique keys).
Comment #26
mikeytown2 CreditAttribution: mikeytown2 commentedFrom http://www.mysqlperformanceblog.com/2013/12/12/one-more-innodb-gap-lock-...
Replace does sound like it could work and it looks like it was never discussed in #937284: DEADLOCK errors on MergeQuery INSERT due to InnoDB gap locking when condition in SELECT ... FOR UPDATE results in 0 rows
db_merge requires stronger isolation than REPEATABLE READ in order to fix deadlocks or did I miss something? http://www.ovaistariq.net/597/understanding-innodb-transaction-isolation... If you could explain this recommendation in detail and weigh the pro's and con's that would be ideal.
Thanks for creating #2232037: taxonomy_index should have a primary key :) Also feel free to give advice on these sets of recommendations: https://groups.drupal.org/node/415883
Comment #27
danblack CreditAttribution: danblack commentedit seems the db cache was worked around in #1167144: Make cache backends responsible for their own storage that performs retries on exceptions. The deadlock was most likely an insert of two identical cache entries at the same time. This will be fixed in #2222635: Waiting for table metadata lock on cache_field table once the cid (or hash there of) isn't a primary (and therefore unique) key.
In essence deadlocks are a consequence of databases not being able to meet the ACID requirements within the relaxed to strict requirements that the isolation level provides. The way to deal with the deadlock really needs to be handled by the application and in the case of cache inserts a retry is appropriate.
I'd suggest the transaction isolation level should be an argument to the db_transaction object that reverts on commit.
Comment #28
mikeytown2 CreditAttribution: mikeytown2 commentedComment #29
pounardI suggest we lower the default transaction level at the BDTNG level as a first step and start thinking how about we need ACID in Drupal as a follow-up. We have to take into account that Drupal database schema is stupid (very very stupid) and doesn't comply to any king of consistency since it doesn't even have any FOREIGN KEY constraints. It doesn't have any constraints at all except PRIMARY KEYs and some sporadic UNIQUE KEY. Let's avoid stupid deadlocks.
Comment #30
danblack CreditAttribution: danblack commented> I suggest we lower the default transaction level at the BDTNG level
From what I've seen of the code it seems to me making a lot of assumptions that a SELECT statement is a perfect consideration of a following UPDATE (e.g. db_merge) and as such assuming a default REPEATABLE-READ is in effect.
Helper fixes that allow things to be fixed after careful consideration:
> Drupal database schema is stupid (very very stupid) and doesn't comply to any king of consistency since it doesn't even have any FOREIGN KEY constraints.
Ok, lets find the instances of these, add a key and and put them in a transaction so at least its an all or nothing change.
> It doesn't have any constraints at all except PRIMARY KEYs and some sporadic UNIQUE KEY. Let's avoid stupid deadlocks.
If you define the constraints of PRIMARY and UNIQUE you shouldn't be surprised if occasionally parallel requests cause inserts of the same primary/unique key. Its not the database problem to say which is right or wrong. It just accepts one and rejects another with an exception.
So lets also avoid dumb deadlock detection and processing (core/lib/Drupal/Core/Cache/DatabaseBackend.php:set where its retried without realizing that its happening because the cid is unique and two threads failed a cache hit at the start and separately created a new entry after generating the same data. The right behaviour here is to catch the deadlock exception and ignore it and not retry a set (are the cache entries really any different?). Alternately #2222635-14: Waiting for table metadata lock on cache_field table that has cidhash as non unique).
Comment #31
pounardI'm not really sure you actually understood what I said.
Comment #32
regilero CreditAttribution: regilero commentedjust a link to https://drupal.org/node/1800286#comment-8672597, where a test is made of parallel DELETE+INSERT on REPEATABLE READ -> deadlock. And delete+insert is the default mode for drupal field updates...
Comment #33
mikeytown2 CreditAttribution: mikeytown2 commentedIn reply to #14/#15 this has been committed to D8 and D7 patch is waiting #610076: Add a primary key to the {taxonomy_index} table and prevent duplicates being inserted
Comment #34
tatyana CreditAttribution: tatyana commentedDoes anyone have any experience running a commerce site with READ COMMITTED?
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commented@tatyana: we have been recommending READ COMMITTED for a very long time. Virtually all the large users of Drupal Commerce are running under READ COMMITTED.
Comment #36
tatyana CreditAttribution: tatyana commentedGreat, thanks for the confirmation
Comment #37
mikeytown2 CreditAttribution: mikeytown2 commentedGetting a Fatal error: Unsupported operand types
Adding this to database.inc seems to fix it
This needed to be adjusted in install.core.inc as well (missing the list dir).
Fatal error: Call to undefined function list_extract_allowed_values()
Comment #38
hefox CreditAttribution: hefox commented#610076: Add a primary key to the {taxonomy_index} table and prevent duplicates being inserted is issue to add primary to taxonomy_index
Comment #39
catchIt's an edge case but one that lots and lots of sites run into quite frequently, and it's a fatal when you do.
Comment #40
colanSo if you're using replication, it looks like you'd need these three (3) settings. The additional two (2) on the end are optional.
It looks like these settings need to be on both master & slaves as per a MySQL Server Asynchronous Replication Example.
References:
I'd add this to the wiki page, but Fixes for MySQL Deadlocks in D7 isn't a wiki page.
Comment #41
mikeytown2 CreditAttribution: mikeytown2 commentedAdd it as a comment; unfortunately I can't change it to a wiki. Thanks for doing more research into this :)
Comment #42
morgantocker CreditAttribution: morgantocker commentedFor some MySQL version context, we proposed changing the default to READ-COMMITTED, but later withdrew the proposal based on feedback received. Here is my blog post in it: http://www.tocker.ca/2015/01/14/proposal-to-change-replication-and-innod...
Dimitri has also published benchmarks on isolation level impact. Repeatable-read performs better with short transactions/queries:
http://dimitrik.free.fr/blog/archives/2015/02/mysql-performance-impact-o...
(I'm still a fan of read-committed.. just noting this change isn't as straight forward beneficial as it seems. Dan's comments about db_merge are also true: there are probably other reapeatable-read assumptions being made.)
Comment #43
catchThis directly conflicts with #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate.
Comment #44
gopisathya CreditAttribution: gopisathya commentedWe recently encountered “Deadlock found when trying to get lock; try restarting transaction: DELETE FROM {cache_field}” in production.
we run following as part of deployment process
> drush updb -y
> drush fra -y
> drush cc all
We use replication.
Is putting following READ-COMMITTED setting in settings.php is safe in production environment (or) we should do at mysql server level as mention by https://www.drupal.org/node/1650930#comment-9407649
$databases['default']['default']['init_commands'] = array(
'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'"
);
Comment #45
rodrigoeg CreditAttribution: rodrigoeg at CI&T commentedWe have also found this issue on Drupal 8 (we tested using 8.0.2), on user registration flow (URL: /user/register or using directly the User functions in core).
The deadlock occurs with the 'users_field_data' table.
In our initial tests, these deadlocks have stopped after changing to 'READ-COMMITTED'.
Comment #46
catchPostponing on #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate.
Comment #48
DamienMcKennaBumping to 8.2.x.
Comment #53
Wim LeersFYI: MySQL made
READ-COMMITTED
its new default in version 5.7.Comment #54
mvc@Wim Leers: https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-lev... says the default is still REPEATABLE READ.
Comment #55
pounardLowering the default transaction level per default is often a bad idea, in theory, if database were fast enough, it should always remain isoled serialisable. I experienced very bad behaviors with commerce due to read commited level that left lots of data in an incoherent state. It should be left to each site developer or infrastructure administrator to decide such a dangerous choice.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #57
Wim Leers#54: you're right, I misread http://www.tocker.ca/2015/01/14/proposal-to-change-replication-and-innod...! That was only a proposal, it was not actually applied.
#55: Very interesting remark! Because Damien Tournoud wrote at #1650930-35: Use READ COMMITTED by default for MySQL transactions:
(He wrote that precisely 4 years ago today!)
Comment #59
mkalkbrennerWe ran into these Deadlocks. I think that READ COMMITTED should be the default isolation level for drupal in general.
In fact sub-systems like Redis, Memcache, APCU, Solr, elastic search, ... are defacto opperating in a "READ COMMITTED" mode and therefore MySQL / MariaDB should be configured consistently. Any code in drupal that relies on REPEATABLE READ should be considered to be wrong.
Comment #60
pounard> In fact sub-systems like Redis, Memcache, APCU, Solr, elastic search, ... are defacto opperating in a "READ COMMITTED" mode
Those are not RDBMS, lowering the default transaction level may also cause data inconsistency issues.
Comment #61
mkalkbrennerIf you run REPEATABLE READ you get a completely different behavior if you store a cache entry during a transaction within the database or for example apcu (which the core enables automatically for some caches if available). With READ COMMITTED you get the same behavior. That's why I argue that any code that relies on REPEATABLE READ needs to be considered to be wrong.
Before Drupal switched to InnoDB and used MyISAM, we defacto had something like READ COMMITTED, too (READ UNCOMMITTED to be precise).
The Deadlock we face at the moment causes a data loss unless you write some code that re-runs the latest request.
Comment #62
catchWe made significant changes to router rebuilding, such that I'm not sure #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate is an issue any longer. So unpostponing this.
Comment #63
klausiCan we just set the default init_commands like this?
Comment #64
mradcliffeI think that the approach taken in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate to improve transaction support in our abstraction layer is better than explicitly setting an isolation level. We would want to allow more complex approaches to transactions as it is really, really limited at the moment, and it makes sense to me to allow developers to change the behavior based on complex database models.
Comment #65
mkalkbrenner@mradcliffe I disgree. As I already stated in #61, any contrib or core module that requires REPEATABLE READ does something wrong and the code isn't portable to another database or cache backends anymore.
If your custom module requires it, you already can set it using the existing low level mechanisms.
Therefore I don't think that an API like proposed in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate should be introduced.
Comment #66
mradcliffeI disagree with the following part of your statement that "code isn't portable to another database", @mkalkbrenner. And the differences with transaction isolation level behavior is precisely WHY a developer may want READ COMMITTED over REPEATABLE READ (or SERIALIZABLE over REPEATABLE READ). Furthermore the entire point of a database abstraction layer (DBAL) is to have a high-level implementation to give us tools to do so in code rather than mucking about directly in SQL. In my opinion this leads to a maintainability nightmare of SQL scripts in custom projects.
The API in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate also does more than just introduce isolation levels, but attempts to bring Drupal's transaction handling in par with other DBAL such as Doctrine. Because we don't have robust transaction handling we ended up with this weird Storage API that explicitly depends on implicit transaction commits, which made it necessary to write a stupid hack around it to get things to work in the pgsql driver.
A robust transaction interface that allows for better transaction commits, rollbacks and isolation levels
1. Makes it easier for core and contrib. database drivers to implement transaction isolation levels i.e. improves our core/contrib db drivers.
2. Improves Developer Experience by allowing custom development to use a common interface i.e. improves maintainability.
3. Helps put our current DBAL on a better playing field i.e. improves marketability.
I don't see how not providing a common interface for developers helps out in the long term.
It is a bit off-topic, and so I think that we should add the #2572283 as a follow-up todo in the patch in #63.
Comment #67
klausiGood idea, added the @todo.
The test run took 48 minutes vs. 49 minutes on 8.7.x-dev, so we might see a tiny performance improvement with this :-)
Comment #68
bojanz CreditAttribution: bojanz at Centarro commentedWe still tell all Commerce sites to run under "READ COMMITTED" (and there's even a patch for adding a warning when it doesn't: #2733675: Warning when mysql is not set to READ-COMMITTED). It's also the first step of every performance engagement we do for a client.
Can't comment on the problems hinted at in #55, we haven't encountered them so far.
Comment #69
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think this issue could use a new summary.
In particular, a list of pros/cons based on what we currently know about Drupal sites running READ COMMITTED or having tried to do so but switched back due to problems with it. For example:
Comment #71
Gogowitsch CreditAttribution: Gogowitsch as a volunteer and at QuoData GmbH Quality & Statistics for Merck KGaA commentedGood news: I applied the patch from #67 to my CI setup and ran the Bash script from the related issue #2833539. The script is just a quick way to reproduce an otherwise intermittent issue. With the Bash script, I used to get tons of these errors "1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {node_field_data}". Thanks to the patch, they are gone. I switched between vanilla Drupal 8.6.10 and with patch applied multiple times and found that the patch consistently fixed my issues.
Without the patch, our CI pipeline quickly fails. With the patch, it consistently succeeds.
My setup: Ubuntu 18.04.1, Drupal 8.6.10 running within Docker via Lando v3.0.0-rc.13, MySQL 5.7.25 within Docker, all with default settings.
Comment #72
catchI don't see how this could be an issue, the transaction level is going to have less impact the less writes there are.
I deliberately did not get this on my local machine, and I run into it on almost every cache clear. Bumping to critical since it's a fatal error during normal site operation that affects regular users.
Comment #74
apadernoComment #75
Ghost of Drupal PastFor other interested parties reading this issue and wondering about taxonomy_index: in D8 it has a primay key. It was added in
1d198fbd68
in 2014 from issue #2232037: taxonomy_index should have a primary key.Comment #76
mike82 CreditAttribution: mike82 commentedFor MySQL 8 it needs to be transaction_isolation instead of tx_isolation. Works fine for me.
Comment #77
neclimdulShould we code this directly in the connection calls if it will break some binary log modes? I don't know the answer but I worry there might be some unhappy people that apply an update and suddenly their site is completely broken.
Comment #78
shahankitb1982 CreditAttribution: shahankitb1982 commentedN/A
Comment #79
shahankitb1982 CreditAttribution: shahankitb1982 commentedFor path #67. If there is an older MySQL version then it is fine.
For the newer MySQL 8 version it should be
transaction_isolation
So check first in MySQL
If above result is empty then use
show variables like 'transaction_isolation'
Accordingly changes needed. Thanks
/drupal/web/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
Comment #80
shahankitb1982 CreditAttribution: shahankitb1982 commentedComment #81
shahankitb1982 CreditAttribution: shahankitb1982 commentedComment #82
Kris77 CreditAttribution: Kris77 commentedI have this error with migrate process: "Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {key_value_expire} (name, collection, value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array...
Can I try patch in #67 or in #81?
I'm using Drupal 8.8.
Thanks...
Comment #84
sokru CreditAttribution: sokru as a volunteer commentedPatch against 9.1.x
Comment #85
rgpublicHmm. Don't why, but since I added "transaction-isolation = READ-COMMITTED" our MariaDB 10.4 Galera cluster starts crashing every other day. Stuff like this is suddenly appearing in the logs and is then repeating endlessly while the DB won't accept any new connections:
Mai 29 11:33:58 yak1 mysqld[2479]: Mutex at 0x55c2e3150200, Mutex DICT_SYS created dict0dict.cc:824, lock var 2
Mai 29 11:33:58 yak1 mysqld[2479]: 2020-05-29 11:33:58 0 [Note] InnoDB: A semaphore wait:
Mai 29 11:33:58 yak1 mysqld[2479]: --Thread 139726468044544 has waited at dict0dict.cc line 880 for 41.00 seconds the semaphore:
Mai 29 11:33:58 yak1 mysqld[2479]: Mutex at 0x55c2e3150200, Mutex DICT_SYS created dict0dict.cc:824, lock var 2
Mai 29 11:33:58 yak1 mysqld[2479]: 2020-05-29 11:33:58 0 [Note] InnoDB: A semaphore wait:
Mai 29 11:33:58 yak1 mysqld[2479]: --Thread 139726627870464 has waited at dict0dict.cc line 8
I never had such a problem with the default ("REPEATABLE-READ"). I've removed the READ-COMMITTED for now. Is this really safe to enable also for Galera clusters?
Comment #86
pounardWhy not handling the transaction properly instead ? If cache, key-value store or other bits are identified as often being the cause of transaction deadlock, the proper SQL way of doing this probably is to let them isolated in REPEATABLE READ or SERIALIZABLE, do a SELECT FOR UPDATE when updating (and prey for your RDBMS to properly do predictive locking when inserting), and retry the transactions when they deadlock or fail at COMMIT. It would be easy to implement for cache or key-value if they are in standalone transactions, it would be extremely difficult to implement when they happen in a larger transaction that does business stuf (entity save I guess would be the most common scenario). It might worth the shot to investigate this thought.
Comment #87
catchWe also discussed using a separate database connection for cache/lock etc. in #2347867: Race conditions with lock/cache using non-database storage - add a non-transactional database connection.
Comment #88
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedHaving read the last few comments, this shold be on needs work as there is deep discussion and heady questions following the latest patch.
Comment #89
Ghost of Drupal PastWhile I do not want to play issue ping-pong there's nothing to work on here, a decision needs to be made..
Did we discuss whether Galera is in scope? This patch, after all, just provides a default. We always provided a sensible default for most of Drupal users and provided override/extension points as necessary. Which this patch also does. Is it worth dropping / holding up this patch for Galera users? Mind you: I am neither for nor against it. I am just asking the question.
Comment #90
pfrenssenI think checking the version number to determine whether to use
tx_isolation
ortransaction_isolation
might be brittle. A more reliable way to know which of the two variables is supported by a given MySQL-alike database is to query the available variables:SHOW VARIABLES WHERE variable_name IN ("tx_isolation", "transaction_isolation");
Comment #93
super_romeo CreditAttribution: super_romeo commentedRerolled for D9.1.
Comment #95
jukka792 CreditAttribution: jukka792 as a volunteer commentedI tried to apply patch #84 to Drupal 9.1.2 but got composer error:
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-05-04/1650930-default-mysql-tra...
The reason why tried to apply is that my migrate says sometimes:
SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction
If I remember right, this patch used to work on some older drupal 9 or 8.
Comment #96
elgandoz CreditAttribution: elgandoz as a volunteer commented@jukka792, you have to use the diff from the merge request !109, from the GitLab repo. You can simply go to the merge request and download as .diff, and then change the extension with .patch.
For simplicity i attached the file here from #94, which I can confirm it applies on D 9.1.2.
Comment #97
neclimdulIt may apply to 9.2 but it doesn't apply to 9.2 because of #3185231: Use combination SQL modes instead of explicit modes so it does need a reroll.
Comment #98
neclimdulI still don't like that this can break replicated sites out of the blue because of a log mode setting that could be hard to change. I guess a site can get around that by adding the following to settings.php but I'm not sure how for them to get from a white screen of death to finding this hack.
Comment #99
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedRe-rolled for 9.2.x
Comment #100
JeroenTComment #101
JeroenTComment #102
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #103
hideaway CreditAttribution: hideaway at jobiqo - job board technology commentedReroll of #67 for Drupal core 9.1.x version.
Comment #104
mhavelant CreditAttribution: mhavelant at Brainsum for Tieto commented$version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
This returns the wrong version for Azure Managed MySQL instances and connections fail as `NO_AUTO_CREATE_USER` was removed in MySQL 8.
See "Azure Database for MySQL server" reports wrong database version.
$this->connection->query('SELECT VERSION()')->fetchColumn();
This used as the fix in the issue above to fix this.
Comment #105
mhavelant CreditAttribution: mhavelant at Brainsum for Tieto commentedHere's an updated version (mainly for 9.1.x). I also removed NO_AUTO_CREATE_USER as it was not in 9.1.x either. If that's required I can add it again.
Comment #106
mkalkbrenner+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -206,8 +206,22 @@ public static function open(array &$connection_options = []) {
+ if (version_compare($server_version, '5.7.20', '<')) {
MariaDB still uses tx_isolation. And its Version numbers are higher, 10.x and above.
So this needs to be respected here.
Comment #107
mkalkbrennerComment #108
apadernoComment #109
ankithashettyRerolled the patch in #107, thanks!
Comment #110
prudloff CreditAttribution: prudloff at Insite commentedHere is a reroll for 8.9 in case someone needs it.
Comment #112
mkalkbrennerWe ran this patch since years on various sites with different Drupal versions. In our use-cases we have massive drush commands with transactions and disabled memory caches. Without this patch you might corrupt the data because you miss the updated cache entries from other processes or page requests.
Comment #113
daffie CreditAttribution: daffie commentedHaving this code in the database open function is something I really do not like. Yes, we had that in the past and I was very happy that we could remove it.
Do we really need to do 2 different things here? One thing for MySQL and one for MariaDB. Can we set them both?
The minimum required version for MySQL is now 5.7.8 and we are here testing for 5.7.20. Maybe the minimum required version for MySQL should become 5.7.20 for D10. Then this check can be removed in D10.
I understand that for most sites out there, this change will be an improvement. I just worry about the sites for which this change will result into errors or very slow sites. Especially for the ones where there is very limited knowledge about Drupal. Hoping that this change will not result in a clusterfuck.
Could we add some links to the MySQL and MariaDB documentation about this settings.
Comment #114
mkalkbrennerWow, more concerns then lines of code ;-)
I agree with this.
This could only be achieved when we split the drivers. Or let the MariaDB driver extend the MySQL driver. I wonder if this would be a smart decision anyway as both systems started to diverge.
Comment #115
daffie CreditAttribution: daffie commentedThis is something that should be done in a new major release.
Comment #116
mkalkbrennerI agree.
So my suggestion is this:
How do we get an approval on this approach from the maintainers. I don't won't spent any time on this when it takes 7 more years for an approval.
Comment #117
andypostCurious why this limited to mysql only, pgsql could be affected too and sqlite can have https://sqlite.org/pragma.html#pragma_read_uncommitted
would be great to update issue summary
Comment #118
daffie CreditAttribution: daffie commented@mkalkbrenner: I have asked @catch on Slack if he could give this issue some guidance, as he is one of the core release managers. He can make the decision. When it is a very big decision, he will bring it to the core committer meeting. @catch is also a core committer. Drupal 10 is to be released in the summer of 2022. It is very likely to be fixed in that release. When we get the ok and do the work.
Comment #119
catchQuestion on the patch first:
It seems to me that both MySQL and MariaDB support the syntax:
SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED
.https://mariadb.com/kb/en/set-transaction/
https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html
Is there a reason we can't use that? Then the discussion about separate drivers and versions might be moot.
If that's not possible:
1. I'm not sure about creating a new driver for this issue, it's cleaner when you look at it, but for example if it does turn out there's a cross-platform way to set this, then we'd have an unnecessary driver sitting around. Much easier to remove an if statement than deprecate a database driver and get everyone to migrate back.
2. If we do need to create a new driver, then that could (and should, so that people can change that before doing a major upgrade) happen in a minor release. The only thing we'd have to restrict to a major release, is removing MariaDB support from the MySQL driver.
3. I don't think the variable name changing is a pressing reason to increase the MySQL minimum version. Obviously we could remove the check if we do though.
Comment #120
daffie CreditAttribution: daffie commentedI have talked to @catch on Slack and he said: "I don't think it needs a committers meeting yet - if we do the $settings option, we can have a follow-up to discuss actually changing the default altogether. I think we should change the default for brand new sites, just not existing ones."
Lets do that.
Removing the tags: ''Drupal 10" and "needs release managers review".
Comment #121
catchfwiw for changing the default, I think it comes under the category of 'disruptive bugfix' - i.e. it would have to be either a minor or major release, and probably requires some warning. It's likely to fix issues on far more sites than it breaks, but it's possible it'll break someone's site somewhere.
One option might be:
1. Add the option in settings, defaulting to the current
2. On sites where the default isn't set, show a notice in the status report that the site is using the default, and it's going to change to READ COMMITTED in Drupal X.x
3. Once we change the default, show a notice in the status report that the site is using the default and that the default is READ COMMITTED.
But we should move this over to the follow-up once it's created.
Comment #122
mkalkbrennerhere's a unified patch as an intermediate step.
Comment #123
daffie CreditAttribution: daffie commentedPatch does not apply.
Comment #124
mkalkbrennerComment #125
apadernoComment #126
mkalkbrennerComment #127
mkalkbrennerComment #128
mkalkbrennerUsing the patch in #127 the transaction isolation level is now configurable during the site installation or in settings.php:
Comment #129
daffie CreditAttribution: daffie commentedI think that for this form element the average sites owner shall needs some info about the options. Which should he/she select and what is the default and what are the differences. But not too much. Something sensible.
Why did you add this option. I am not saying that you are wrong just curious.
We are gong to need some tests to make sure that the patch does what we want it to do:
1. That when we have an existing site and the setting isolation_level is not set, that it is using the isolation level REPEATABLE READ (query the database);
2. When we install a new site the key isolation_level is set in settings.php and its value is READ COMMITTED;
3. When we install a new site that it is using the isolation level READ COMMITTED (query the database);
4. When we have an existing sites and the key isolation_level is set in settings.php, that it is using the isolation level that is the same as in settings.php. Test for every possibility (at least for READ COMMITTED and REPEATABLE READ) (query the database).
Comment #130
catchI don't think we should offer this as an option on the form at all - default to READ COMMITTED, those who need something else can also edit settings.php
Comment #131
mkalkbrennerLike this?
Comment #132
daffie CreditAttribution: daffie commentedI think that is what @catch wants. For the tests, you could look at the ones in the directory core/tests/Drupal/FunctionalTests/Installer/.
Comment #133
shaktikfrequently getting errors while creating node and edit.
Comment #134
Sseto CreditAttribution: Sseto commentedOn my D8 site, I had the same issue with deadlock and I added
'isolation_level' => 'READ COMMITTED',
to my settings.php and it worked!Comment #135
timohuismanWe use composer patches to apply patches. Merge request patch urls are updated for every commit on the branch, which makes it a moving target. Attached is a patch containing the current state of the merge request (https://git.drupalcode.org/project/drupal/-/merge_requests/109.patch).
Comment #136
Youcanlearnit CreditAttribution: Youcanlearnit as a volunteer commented#122 didn't work and #124 is empty
Is there a patch to 9.2.5 ?
Comment #137
jukka792 CreditAttribution: jukka792 as a volunteer commentedI am getting this error with D9.2.6 and patch #109
Drupal\Core\Entity\EntityStorageException: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction: INSERT INTO "cache_entity"
I am deleting media content with multiple browser tabs (bulk delete)
Comment #138
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled patch. Please review.
Comment #139
neclimdulstill no settings.php control.
Comment #140
JeroenT#131 adds the possibility to set the isolation_level in settings.php.
I tried to add test coverage that checks if the isolation_level is correctly added to the database settings when a new Drupal site is installed and when a site is installed with existing database settings.
Let's see what the testbot thinks.
Comment #142
JeroenTComment #144
JeroenTComment #146
DieterHolvoet CreditAttribution: DieterHolvoet as a volunteer commentedI rerolled the patch for 9.4.x.
Comment #148
catchLooks like a few more tests need updating to deal with the new form element.
I started on updating the issue summary.
Comment #149
JeroenTComment #150
jibranThis comment is more than 80 lines.
Given it is just a value now we need to update the screenshot in the IS.
Also, do we need to add this option to default.settings.php as per the comment here?
I think we should add a change notice about this change.
Comment #151
daffie CreditAttribution: daffie commentedShould we not add more options here? There are 4 optuions according to MySQL. See: https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-lev...
Now new sites are forced to use "READ COMMITTED" and existing sites are defaulted to "REPEATABLE READ". I would like to give sitebuilders more options.
To much indentation (2 spaces).
Existing sites do not have this line in their setttings.php, therefor needs this line be removed.
This line should test that the "isolation_level" is not set in the settings.php.
What needs to be added is to check the transaction isolation from the database. It should be "REPEATABLE READ". The query should be:
SELECT @@SESSION.transaction_isolation
. See: https://dev.mysql.com/doc/refman/5.7/en/set-transaction.htmlBoth tests are MySQL only and therefor need to be moved to the mysql module.
We have now tests and therefor we no longer need manual testing.
Comment #152
daffie CreditAttribution: daffie commentedUpdated the IS and created the CR.
Comment #153
daffie CreditAttribution: daffie commentedComment #154
daffie CreditAttribution: daffie commentedComment #155
murilohp CreditAttribution: murilohp at CI&T commentedHey! I tried to address both #150 and #151 here, so regarding #150:
sites/default/default.settings.php
documenting the new settingisolation_level
Thanks to @daffie, we have now a CR!
Moving to #151:
READ COMMITTED
option will be the default and now the users can change this onsettings.php
, to help the users I followed the suggestion on #150.2 and documented this new option atdefault.settings.php
, we just need to update the IS removing the screenshot.isolation_level
is not set in thesettings.php
. I also added a test to validate thetransaction_isolation
, thanks for the query!Thanks!
Comment #157
daffie CreditAttribution: daffie commented@murilohp: Thank you for working on this.
sites/default/default.settings.php
you also need to make tocore/assets/scaffold/files/default.settings.php
This form needs to have all 4 possible transaction isolation level options in it. We should also add a text with something like: "The recommended option is 'READ COMMITTED'. Existing sites without this setting will default to 'REPEATABLE READ'. THe other 2 options are available, but not supported. Use them at your own risk. For more info: https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-lev...."
I think we should do a test to see if the set isolation level is one of the 4 possible options. If not, we should throw an new DatabaseTransactionIsolationLevelException. Just before the selected code.
Checking for
isolation_level
is enough. We are testing that the isolation_level key is not set.The part
, $connection_info['default']['init_commands']
can be removed.Can we change this text to: "For MySQL, MariaDB or equivalent databases can the option 'isolation_level' be set. The recommended option is 'READ COMMITTED'. Existing sites without this setting will default to 'REPEATABLE READ'. The other 2 options are READ UNCOMMITTED and SERIALIZABLE. They are available, but not supported. Use them at your own risk. For more info: https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-lev...."
We are missing the option "READ COMMITTED".
Too much indentation. Please remove 2 spaces from the start of every line.
Comment #158
catchI'm not convinced it's useful to add this to the installer form, it seems like an invitation for people to shoot themselves in the foot. Just making sure we have decent documentation in default.settings.php seems enough here.
Comment #159
murilohp CreditAttribution: murilohp at CI&T commentedHey I addressed #157 on this new patch.
default.settings.php
DatabaseTransactionIsolationLevelException
for this and a new kernel test to validate this exception.'isolation_level'
string inside bothsites/default/default.settings.php
andcore/assets/scaffold/files/default.settings.php
as a comment, how do we handle this? I mean I think this test will fail here.READ COMMITTED
Leaving as NW to see the testbot here. Also, it would be nice to have your thoughts here @daffie.
Comment #160
daffie CreditAttribution: daffie commentedI am going to work on the tests.
Comment #161
daffie CreditAttribution: daffie commentedI have updated the tests and made a small improvement to MySQL's install Tasks class.
Comment #162
andypostIt looks ready to go, failures are unrelated and known
+1 rtbc
Comment #163
mkalkbrennerComment #164
alexpottGiven these are not supported I would not have the specific PHP to set them here.
Tempting to use a value enum here on Drupal 10. Then we wouldn't need the exception. It would mean a divergence between Drupal 9 and 10 but maybe that is okay.
I've got a big concern here. This is going to result in unnecessary work on a lot of sites that are already using READ-COMMITTED via MySQL configuration. Are we sure that this is the correct approach here? I think we should use a different approach here: I think we should only set the isolation_level if it is set. Further more, and crucially, if this change goes in as planned it is going to change ALL isolation levels to REPEATABLE READ - which feels super wrong. Many sites already default to READ-COMMITTED via my.cnf or other configuration options.
Comment #165
mkalkbrennerYou're right. So the correct fix will be to set READ-COMMITTED per default for Drupal 10. In Drupal 9 the isolation level should only be set if explicitly configured in settings.php?
Comment #166
daffie CreditAttribution: daffie commentedFixed all points from @catch in comment #164. @catch: Thank you for the review.
For D9 the isolation level is only set when if explicitly configured in settings.php
For D10 the 4 isolation levels are created without enumeration. The PHPCS process does not like PHP 8.1 enumeration. The testbot result is:
The patch can be found here: #3264683-11: [IGNORE] Testing only
PHPCS is working on support for PHP 8.1 stuff: https://github.com/squizlabs/PHP_CodeSniffer/issues/3479.
@mkalkbrenner: Thank you for your input.
Comment #167
daffie CreditAttribution: daffie commentedForgot to remove core/modules/mysql/src/Driver/Database/mysql/IsolationLevel.php.
Comment #168
alexpottI think we can do $isolation_level = IsolationLevel::from()... that will throw an exception for us that probable is quite descriptive. That said given coder needs fixing for enums then perhaps we should drop that and covert to enums later.
This still means for every connection to the db we have to send additional commands. Which will result in additional network traffic if the database is not local - I don't know if this matters...
Comment #169
daffie CreditAttribution: daffie commentedFor #168.1: As I explained in #166, we cannot do enumerations, because it will fail PHPCS.
For #168.2: I do not know how else to do it. I will look into it.
Comment #170
catchI think we should open a 10.1 clean-up issue for enum, no need to add work to a decade old critical bug.
Comment #171
alexpottDiscussed with @catch and we agreed to promote #2733675: Warning when mysql is not set to READ-COMMITTED to critical in-lieu of this one. And postpone this issue on that one.
Setting this on all databases is potentially a small performance regression on sites where it is already set and changing it automagically is a bit fraught as discussed above. So we should do #2733675: Warning when mysql is not set to READ-COMMITTED to alert people and give good instructions about ways to set it. And then come back to this issue once we've landed that one. Once we have this on the status report I don't think this issue is critical any more.
Comment #172
Steven McCoy CreditAttribution: Steven McCoy commentedWhat would be involved in creating a patch that works for the current stable Drupal (9.39 at the time of writing)
Comment #174
Freddy RodriguezNot sure if this module was meant to fix and optimize transactions in D7: https://www.drupal.org/project/apdqc
Is it worth migrating this to d9/d10?
Comment #175
daffie CreditAttribution: daffie commented#2733675: Warning when mysql is not set to READ-COMMITTED has landed.
Comment #176
daffie CreditAttribution: daffie commentedComment #177
daffie CreditAttribution: daffie commentedTalked to @catch on Slack and this issue can become a major one.
Comment #180
claudiu.cristeaI've created a new MR (the old need base branch change by @super_romeo) on top of
9.5.x
. Rerolled, #166.Agree with @catch on #170, enum issue should be a follow-up
Comment #181
daffie CreditAttribution: daffie commented@claudiu.cristea: Thank you for working on this.
Comment #182
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedAddressed the comment on #181
Attached Patch and interdiff
Comment #183
alexpottI'm not convinced we should be fixing this issue. We have the warning and there are plenty of ways to set the default if you want to. Like you can change you MySQL server to default to 'READ COMMITTED' for all transactions. For example you could run
SET GLOBAL TRANSACTION ISOLATION LEVEL READ COMMITTED
and be done.I don't think this check is actually worth it. If you set it to something bogus mysql is going to fail anyway. And this is run on every request. I'm not sure it is that useful.
Why are we setting it twice here? Once in the sql_mode and once in the isolation init command. I think the sql_mode changes are wrong. I think #181.2 is suggesting we make sql_mode two queries... hmmm. That'll mean using a semi-colon in the query. This will probably work because we are using \PDO::exec() directly here. Not sure if it is the right thing to do.
As above I'm not sure this exception is actually worth it.
Comment #184
mkalkbrennerHere's the patch against Drupal 9.4.1 as we need it. It addresses Alex' concerns in 183 I agree with.
I'm still of the opinion the the default has to be changed and that the patch is needed to do that. The argument have been mentioned in this issue already.
Comment #185
daffie CreditAttribution: daffie commented@mkalkbrenner: You patch is missing some code changes and it should be for D9.5. Also could you an interdiff file. It makes reviewing your changes a lot easier.
Comment #186
alexpottYou can't do this. SET variable and SET TRANSACTION do not mix.
We can do
"SET sql_mode = 'ANSI,TRADITIONAL'; SET TRANSACTION ISOLATION LEVEL " . $connection_options['isolation_level']
- to make this a single query to send to the db server.But I still feel that we shouldn't be setting the default here. I'm not sure that Drupal is the correct place to configure this. Hence we now have the warning.
Comment #187
mkalkbrennerYour right. I forgot to add the additional tests.
The interdiff reports "Whitespace damage detected in input" against #182.
I assume that others are in the same situation as we are and therefore require a patch against Drupal 9.4 to fix their broken deployment because of the release of 9.4.x. So this is my priority. The patch for 9.5.x might follow later.
Comment #188
mkalkbrenner@alexpott see my comments #59 and #61.
Comment #189
alexpottFor me if #59 and #61 are reasons then we need to do this also for SQLite and Postgres or at least detail why this is not going to cause different behaviour for them which will be even more confusing than the current behaviour - which has been this way for years. I don't think the reasoning given here is good enough to change the default behaviour. I think we need to do #3291949: Improve the MySQL transaction isolation warning because we should not be producing warnings on things that are supported. I think there are good reasons why you might want READ COMMITTED - but there are also good reasons for REPEATABLE READ.
Comment #190
alexpottWell look at that Postgres default is read committed... hmmm. That is interesting. SQLITE is a different beast altogether and does not have a read committed mode. So I guess we can argue that setting it to read committed is making our approach to db drivers more consistent. Another point in this changes favour is that Drupal core and all contrib is tested with READ COMMITTED because that's the default on DrupalCI.
What makes me nervous about this change is that there is some site out there which relies on the extra compliance of REPEATABLE READ for something financial and by making this change we introduce a very hard to solve and find bug.
Comment #191
daffie CreditAttribution: daffie commentedWe can make sure that this change is the first to be mentioned in the Drupal 9.5 release notes.
As an alternative we can make the change only for new sites and at some later date we can change the setting for existing sites.
Comment #192
mkalkbrennerI just got a report from my colleague. Since we moved to Drupal 9.4.0 in dev we had to remove the patch from here because it didn't apply anymore. As a consequence our heavy migration scripts to migrate an old legacy system to Drupal ran into
[error] Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock;
multiple times.Now with patch #187 applied it works again. We use MariaDB.
Comment #193
mkalkbrennerComment #194
alexpott@mkalkbrenner the thing I don't get is why you wouldn't fix this at the database config level instead of needing a drupal patch. Like you can set this in your my.cnf so your database connections are in READ COMMITTED by default and you'll be good to go with no patch and less risk.
Comment #195
catchThe issue summary still says we'd set this for new sites only, I think changing it for existing sites should be another follow-up probably - should be a much smaller patch but with a lot more consequences.
Comment #196
mkalkbrennerI'm still convinced that not using READ COMMITTED is a bug. I want to get rid of all these reports about dead locks or cache inconsistencies in contrib or core and the common advice to "clear your caches". It's not just about our current project at Jarltech.
New users should not search existing issues for a solution of a problem they run into (if they get more traffic). It should work out of the box.
Comment #197
mkalkbrennerComment #198
daffie CreditAttribution: daffie commented@mkalkbrenner This is D9.5 and newer issue. Only bugfixes can go into a already released version (D9.4).
Comment #199
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedI was curious what Drupal-focused hosting providers are using. It looks like Pantheon is using READ COMMITTED, and Acquia is using REPEATABLE READ. It doesn't look like Pantheon documents the setting anywhere. In that case, it seems to me that the risk of production regressions changing this is fairly low. Otherwise, I expect Pantheon would have documented this for customers migrating to them.
Comment #200
rgpublicFWIW: I wrote 2 years ago in this thread that READ COMMITTED caused us problems on Galera. This seems to have resolved as in the meantime we were audacious enough to switch it back on and we are using READ COMMITTED for quite some time now without any further problems. So, basically good news on that front.
Interestingly, Galera is still warning about using this:
https://galeracluster.com/library/documentation/isolation-levels.html
(See the red box under "Understanding Isolation Levels")
I wonder what they mean with "In multi-master mode, however, you can only use the REPEATABLE-READ level.". We are on using multi-master mode and SHOW VARIABLES LIKE '%TX%' shows that READ-COMMITTED is enabled. Is this perhaps automatically ignored? Is this information still valid? Could there be any lurking dangers for data consistency that I'm not aware of?
At least I can say that it hasn't proven totally disastrous for us and we are running about 50 Drupal websites approx.
Comment #201
mkalkbrennerIt seems that the discussion came to an end as that the community agreed on READ COMMITTED:
#2733675: Warning when mysql is not set to READ-COMMITTED
So the remaining issue for Drupal 9.5 and 10.0 is whether to explicitly set the default or not.
And it would be great to ensure that the tests on drupal.org are using that mode!
Comment #202
Ghost of Drupal PastSo to sum up the problem here, as far as I understand these matters: READ COMMITTED can only be used safely if every table has a primary key because otherwise InnoDB might issue a full table scan on the slave for every row modified which can lead to abysmal slave performance or even a crash. See for example https://bugs.mysql.com/bug.php?id=53375 and https://bugs.mysql.com/bug.php?id=76252
Both MySQL and MariaDB can enforce PK: MariaDB 10.1 added https://mariadb.com/kb/en/innodb-system-variables/#innodb_force_primary_key but that's for InnoDB only and MySQL 8.0 added https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sys... . Drupal doesn't require MySQL 8.0 which is a pity for other (window function) reasons but I digress.
So I think we should have an issue which adds a warning to the status report if there are tables in the schema which doesn't have a primary key.
Comment #203
daffie CreditAttribution: daffie commentedNo interdiff file, because the old patch did not apply any more. Based on the patch from comment #182:
Users can also select to not let the transaction isolation level be set by Drupal and set it manually in the database.
@chx: Greart point. I will into it.
Comment #205
jedgar1mx CreditAttribution: jedgar1mx as a volunteer commentedAnyone having issues installing this on 9.4.1? I keep getting
Could not apply patch!
error.Comment #206
Steven McCoy CreditAttribution: Steven McCoy commented@jedgar1mx - I'm using the patch from #149 on 9.4.1
Comment #208
andypostOnly one test fails
Drupal\Tests\mysql\Functional\RequirementsTest
, for some reason the message is not displayedComment #209
acbramley CreditAttribution: acbramley at PreviousNext commentedWe were hitting deadlocks on
cache_bootstrap
writing the last_write_timestamp when we had >1 user while running a K6 load test that was creating an entity. While I'm surprised it's that easy to cause a deadlock, I can happily say that swapping to READ COMMITTED allowed us to bump up to over 100 concurrent.The call stack for writing this key is a bit suspect however:
Why does it need to mark something outdated in cache_bootstrap when invalidating entity tags?
EDIT: It also does the same for cache_config and cache_discovery. None of these bins contain any of the tags being invalidated.
Comment #210
Jan-E CreditAttribution: Jan-E commentedCould it be to mitigate https://www.drupal.org/project/drupal/issues/2833539 ?
Comment #211
acbramley CreditAttribution: acbramley at PreviousNext commented@Jan-E not sure what you mean? This invalidation is causing the deadlocks.
Comment #212
catch@acbramley Cache tags can be used with any cache bin - for example the render cache bin will have more entity cache tags in it than the entity cache bin.
However, I don't think we need to invalidate the entire fast backend in that case, so I've opened #3334489: ChainedFastBackend invalidates all items when cache tags are invalidated to discuss more.
Comment #213
acbramley CreditAttribution: acbramley at PreviousNext commented@catch awesome! Thank you :)
Comment #214
daffie CreditAttribution: daffie commentedFixed the Drupal\Tests\mysql\Functional\RequirementsTest.
Comment #215
gidarai CreditAttribution: gidarai at Finalist commentedAdded a test to check if all tables have a primary key (if not show a warning on the status report page)
Comment #216
gidarai CreditAttribution: gidarai at Finalist commentedAdded the patch and interdiff files (didn't add it before)
Comment #217
gidarai CreditAttribution: gidarai at Finalist commentedFixed code standards
Comment #218
gidarai CreditAttribution: gidarai at Finalist commentedRegenerated phpstan baseline
Comment #219
alexpott$this->t() is available and preferred to t()
I think we need to have a link to something that explains what this is and the text needs finessing to be more user friendly. I also think the text "Do not set by Drupal" needs works to. It's always better to have a positive than a negative. So something "Leave to database server configuration" would be better.
Need to use translatable strings here. I'm not convinced this requirement should be shown if there are no missing primary keys. But also should we be doing this check if the isolation level is set to REPEATABLE-READ?
Also if you do have tables without a primary key what can you do? If they are your custom modules then fine but if they are a contrib module or core you going to need to be told to create an issue.
Comment #220
larowlanYes, excellent point, I think we need to provide a way for the site owner to action this.
Comment #221
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAddressed the comment #219 as commented some changes required, so upload patch against #218 and inter_diff file along with it. Please review.
Comment #222
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #223
acbramley CreditAttribution: acbramley at PreviousNext commentedReroll of #221, there was a slight fuzz on the phpstan baseline changes.
I think we are still missing all changes in #219.2 and 219.3
Comment #224
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #225
acbramley CreditAttribution: acbramley at PreviousNext commentedLets try that again....
Comment #226
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #227
acbramley CreditAttribution: acbramley at PreviousNext commentedHuh?! These patches are being rolled directly from 10.1.x-dev. I don't know how they keep failing to apply.
Comment #228
jungleRejected hunks, FYI
Rerolled from #223
Comment #229
jungleComment #230
acbramley CreditAttribution: acbramley at PreviousNext commentedHmm, very weird. It looks like my local clone was stuck on a commit from Feb 7. Even updating the remote didn't work. Cloning fresh has fixed it, however #229 still doesn't apply
EDIT:
git apply
does work,patch -p1
doesn'tEDIT 2: Fixed by installing gpatch
brew install gpatch
Comment #231
jungle[DELETED]
Comment #232
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch #229 no longer cleanly applies to D10.
Comment #233
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch at #229 against 10.1.x.
Comment #234
apadernoComment #235
gidarai CreditAttribution: gidarai at Finalist commentedFixed patch where custom commands were failing
Comment #236
gidarai CreditAttribution: gidarai at Finalist commentedAdded different warnings for different isolation levels and when primary keys are missing it shows the corresponding tables that do not have a primary key.
Updated the testIsolationLevelWarningNotDisplaying test to check if the proper warnings/errors are displaying on the status report page.
Comment #237
gidarai CreditAttribution: gidarai at Finalist commentedCustom commands failure fix
Comment #238
daffie CreditAttribution: daffie at Finalist commentedThis variable is not necessary. We can also use empty($tables_missing_primary_key).
Can we rename this variable to
$tables_missing_primary_key
Can we put these 3 lines on a single line. Can we test $connection_options['isolation_level'] with strtoupper(). Then when a user sets the variable with "read committed" it also works.
This test is no longer necessary.
This change is a good one, only out of scope for this issue.
This change is also out of scope for this issue.
Comment #239
gidarai CreditAttribution: gidarai at Finalist commentedmade changes suggested by @daffie
Comment #240
gidarai CreditAttribution: gidarai at Finalist commentedFix custom commands failing
Comment #241
gidarai CreditAttribution: gidarai at Finalist commentedmoved $isolation_level line up
Comment #242
harivansh CreditAttribution: harivansh at OpenSense Labs commentedComment #243
gidarai CreditAttribution: gidarai at Finalist commented@harivansh you seem to have made changes that will break the code as you're trying an array to string conversion. Furthermore the if statement checks if the missing primary key array is not empty to determine whether you are missing primary keys. Please take a better look at the issue next time before making changes and test them accordingly.
Comment #245
daffie CreditAttribution: daffie at Finalist commentedAll code changes look good to me.
Testing has been added.
The IS and the CR are in order.
For me it is RTBC.
Comment #246
larowlanUnless I'm mistaken, these strings cannot be translated - but should be able to be.
Should this just be else? do we need to specify the unsupported levels?
I think this should be 'Do not set using Drupal' rather than by. Happy to hear otherwise/counter arguments.
DO NOT SET BY DRUPAL is the key, not the displayed user string, we should use the displayed user string
I think we should assert the output contains test_table_without_primary_key so we don't get a false positive here from some other future test module with a table that has no primary key
Comment #247
mstrelan CreditAttribution: mstrelan at PreviousNext commentedRe: 246.2
It can't be
else
because that would also catch$isolation_level == 'READ-COMMITTED'
where there are no tables missing primary keys which should not be an error or warning.Comment #248
longwaveAddressed #246. I made all the strings translatable, refactored the if statement in
mysql_requirements()
, simplified the descriptions a bit, changed the "Do not set by Drupal" text to "Use database default", and updated the tests to match.Comment #249
daffie CreditAttribution: daffie at Finalist commentedAll remarks of @larowlan have been addressed.
Back to RTBC.
Comment #252
catchRe-read the patch end to end and also the interdiff in #248. I think this is in a good spot, and it's good timing to get it into 10.1.x before the alpha.
Also double checked our docs for the connections options in default.settings.php, and those are still fine without any changes. This patch is just adding installer support and improved status report messages, everything else has been prepared in other issues in the meantime.
Fixed these two nits on commit:
option
option
Did my best with commit credits, but that's challenging with 250 comments from dozens of people, so apologies for anything overlooked.
Committed/pushed to 10.1.x, thanks!
Comment #253
catchComment #254
longwaveTagging for release notes.
Comment #255
daffie CreditAttribution: daffie at Finalist commentedGreat that this is fixed!
It took us only 10 years. ;-)
Thanks to everybody who made this happen!
Comment #257
neclimdul#3362726: READ COMMITTED requirement check doesn't account for database views
Filed a follow up because the requirements check doesn't take into account how mysql treats views as a table so registers warning/error if you have any in your database because a view can't have a primary key.
Comment #258
larowlanQuick followup #3365945: Errors: The following table(s) do not have a primary key: forum_index
Comment #259
Nicolas Bouteille CreditAttribution: Nicolas Bouteille commentedHello everyone,
My website currently still uses the REPEATABLE READ transaction isolation level.
I would like to switch to READ COMMITED.
I read this documentation :
https://www.drupal.org/docs/getting-started/system-requirements/setting-...
From what I understand, for best performance "The preferred way to change the transaction isolation level" is to set the Global value on the server side with
SET GLOBAL transaction_isolation='READ-COMMITTED';
However, if I'm not mistaken, this global variable is shared between all databases of the server. Plus updating this variable needs to be done with a super user.
Where I work, we have multiple databases of different websites on the same server. Plus my database user cannot update this variable. So if I decided to update this variable globally, I would need to coordinate with all other websites and ask IT to do it for me.
For that reason, updating it in settings.php would be easier for me.
In the documentation, the syntax presented in settings.php is the following:
BUT there's a warning: "Adding the setting of the transaction isolation level to the init commands in the settings.php file has the disadvantage that on every page request the transaction isolation level is set. That is an extra database call for every page request!"
Since my goal here is to improve performances, after reading this, configuring this in settings.php was not an option for me anymore.
But then I realized that I actually read here that "The transaction isolation level can be set in the database settings array in setttings.php" and that "Drupal now sets the READ COMMITTED transaction isolation level by default for new installs on MySQL."
So I wondered… if setting the isolation level inside settings.php is bad for performance because it redefines the level for each db request, how come Drupal now sets it by default inside settings.php??
I actually created a brand new Drupal website recently, and it uses indeed READ COMMITED. So I went to the settings.php but to my surprise, the way the isolation level is defined is not
but simply :
'isolation_level' => 'READ COMMITTED',
Here are my many questions:
Is the documentation outdated?
About this new syntax ('isolation_level' => 'READ COMMITTED',): does it have better performance than the 'init_commands' SET SESSION tx_isolation=\'READ-COMMITTED\ syntax? Or is it just a simpler way to write it but in the end it does the exact same thing and has the same drawback of setting the isolation level for each session?
How bad is it to actually set the transaction isolation level on every page request?
Should I do all I can to set the transaction isolation level globally on my database server and avoid to set it in settings.php whenever possible? Or I should not care about this because I won't see a difference...?
In settings.php, is the parameter 'isolation_level' => 'READ COMMITTED', added to every new Drupal site? Or is there a check that is made to the server at site creation and 'isolation_level' => 'READ COMMITTED' is added to settings.php only if the global isolation level of the server is not already READ COMMITTED?
What happens if the parameter 'isolation_level' => 'READ COMMITTED' is still present on settings.php but the DB serveur global isolation level is updated to READ COMMITED later on. Will the isolation level still be redefined for each session no matter what, for nothing? In other words, once we set READ COMMITED globally on server side, should we remove the 'isolation_level' => 'READ COMMITTED' line from settings.php to improve performances?
Thank you in advance for your feedback :)