Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The access check looks if the node has more than one revisions, the query that it executes for that is not indexed:
mysql> select count(*) from node_revision;
+----------+
| count(*) |
+----------+
| 172996 |
+----------+
mysql> SELECT sql_no_cache COUNT(*) FROM node_field_revision WHERE nid = N AND default_langcode = 1;
+----------+
| COUNT(*) |
+----------+
| 1 |
+----------+
1 row in set (0.14 sec)
mysql> explain SELECT COUNT(*) FROM node_field_revision WHERE nid = N AND default_langcode = 1;
+----+-------------+---------------------+------+-----------------------+-----------------------+---------+-------+-------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+---------------------+------+-----------------------+-----------------------+---------+-------+-------+-------------+
| 1 | SIMPLE | node_field_revision | ref | node_default_langcode | node_default_langcode | 4 | const | 86881 | Using where |
Proposed resolution
extend the node_default_langcode index to include NID?
mysql> SELECT sql_no_cache COUNT(*) FROM node_field_revision WHERE nid = N AND default_langcode = 1;
+----------+
| COUNT(*) |
+----------+
| 1 |
+----------+
1 row in set (0.00 sec)
mysql> explain SELECT COUNT(*) FROM node_field_revision WHERE nid = N AND default_langcode = 1;
+----+-------------+---------------------+------+-----------------------+-----------------------+---------+-------------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+---------------------+------+-----------------------+-----------------------+---------+-------------+------+-------------+
| 1 | SIMPLE | node_field_revision | ref | node_default_langcode | node_default_langcode | 8 | const,const | 1 | Using index |
Remaining tasks
User interface changes
API changes
Beta phase evaluation
| Issue category | Bug because of performance |
|---|---|
| Issue priority | Major because of performance |
| Prioritized changes | The main goal of this issue is performance |
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | slow-query-in-2261669-38.patch | 11.3 KB | jhedstrom |
Comments
Comment #1
berdirRaising to major, this query is really slow. Only for users who can actually access revisions, but still. On some sites, that's all, like drupal.org.
I was hoping that #808730: Show the Revisions tab/page even when only one revision exists. would remove the query, but it doesn't look like it.
Comment #2
berdirTwo patches, one adds a new index, the other ads nid to the existing one.
According to http://dba.stackexchange.com/questions/87556/mysql-multiple-column-index..., the order shouldn't matter too much, so having only one index is probably better?
Comment #3
catchAdding to the existing index is good. The default_language index would be completely redundant with the new one anyway.
Comment #4
berdirNo, it would't be. (default_langcode) would be redundant with (default_langcode, nid) but it is not redundant with (nid, default_langcode).
But yes, I guess extending the existing one is fine, as you said, we save the storage of an additional index.
Comment #5
plachNot sure what
node__default_langcodeis currently buying us, actually. Same goes fornode_field__langcode. Would about replacing them with a[nid, default_langcode, langcode]index? I noticed we have no index onnidinnode_field_revision, which sounds baad.Comment #6
berdirThat works to for me, that would be generic enough to add it by default to all revision/data tables, actually, should we do that?
And doing it generic also means that the NOT NULL additions should be generic...
update.php runs through partially, but seems to fail on those with data. I've seen just adding indexes work, maybe that's because of the NOT NULL change?
The diff between the schema for the node tables is:
Comment #8
berdirUsing the key....
Comment #10
berdirFixing the unit tests.
Comment #11
berdirA review would be great :)
Comment #12
jhedstromAdded a beta phase evaluation. Reviewing now.
Comment #13
jhedstromThis patch works as expected. Not sure if there's anything left to do before RTBC.
Comment #14
plachIn general looks good to me, I'm wondering whether we could get rid of
ENTITY_TYPE_field__langcodetoo.Can we name this
$entity_type_id . '__id__default__langcode'?Comment #15
berdirI'm not sure what you mean with
ENTITY_TYPE_field__langcode. __langcode only exists three times in the code with this patch, and it's all added here for the new index.Renamed.
Comment #16
wim leersLooks sane to me. But I'm no SQL expert, so not RTBC'ing.
Comment #17
plachSorry for losing track of this
It's not touched by this patch, but if you have a look to
node_field_datayou will notice anode_field__langcodeindex that IMO could be dropped too.Comment #18
berdirOk, removed that.
I think we have an issue to allow index updates on entity types with data? Somehow that doesnt' seem to quite work. I think it works when it is defined by the field, but not here..
Wondering if we should try to fix that first, to make this less of a problem for existing sites. I'll try to dig that issue out.
Comment #19
berdirComment #20
berdirFound it :)
#2340993: SqlContentEntityStorageSchema::requiresEntityDataMigration() returns TRUE for cases where it should return FALSE
Works great in combination with this, can switch back and forth without problems.
Comment #21
berdirNice, that's in already!
Which means this issue comes with a working upgrade path :)
Comment #22
plachCool :)
Comment #23
alexpottWhat query are we adding the langcode to the index for?
For consistency shouldn't this be
__id__default_langcode__langcode?Comment #24
plachThe new index could cover queries like "give me all nodes whose original language is English". We prioritize the default flag, as we already have the primary key to cover plain language conditions, and we have many queries in core having entity id + default language condition.
Yep, technically that's more consistent, I suggested the current value as it's more compact and IMO conveys more or less the same information at a first look.
Comment #25
timmillwoodHad to manually apply the patch as it was so old so hope I got everything, also updated the suggested key in #23.
Comment #27
timmillwoodForgot to update the tests to __id__default_langcode__langcode
Comment #28
timmillwoodHelps if the patch has some code in it.
Comment #29
timmillwoodawesome, a passing test.
Comment #30
googletorp commentedCode looks good. Changes can be applied on a test database with some random content, so all looks good to me.
Comment #31
catchI think entity storage will handle this automatically, so it shouldn't need a hook_update_N().
However we should be able to test that the new index has been created successfully using db_index_exists() in an update test.
Comment #32
jhedstromI'll write the test.
Comment #33
jhedstromThis adds a test. The only downside to adding this is that it will start failing if the test database is ever updated to be *after* this patch is committed. However, the test could simply be removed at that time.
Comment #34
timmillwood@jhedstrom Awesome! Thanks for the tests.
Looks good to me, and passes.
The failure in Postgres seems unrelated to this patch so assuming Postgres tests are not quite ready yet.
Comment #35
andypostAny reason to make the name so much long?
Comment #36
timmillwood@andypost - I was a suggestion from @alexpott in #23 for consistency.
Comment #37
plachLooks great, but should we check also the removed indexes in the test?
Comment #38
jhedstromThis tests for the previous index on
node_field_datato be properly removed.Comment #39
catchThanks test additions look great.
Comment #40
jhedstromI don't think the commit was pushed.
Comment #41
catchSorry I meant to RTBC, not mark fixed...
Comment #42
catchBut since I'm here.
Committed/pushed to 8.0.x, thanks!