- π©πͺGermany mkalkbrenner π©πͺ
We just ran into a similar issue when upgrading from Drupal 10.2.x to Drupal 10.3.0. The DB update took very long, 16min 23sec.
So I monitored the database queries during the update and found thousands of similar queries that could not use an index and lead to full table scans:SELECT "s".*, "t"."language" AS "language", "t"."translation" AS "translation", "t"."customized" AS "customized" FROM "locales_source" "s" LEFT OUTER JOIN "locales_target" "t" ON t.lid = s.lid AND t.language = 'fr' WHERE "s"."lid" IN (SELECT "l"."sid" AS "sid" FROM "locales_location" "l" WHERE ("l"."type" IN ('configuration')) AND ("l"."name" IN ('core.entity_form_mode.simplenews_subscriber.page')))
+------+--------------+-------------+--------+---------------+--------------+---------+----------------+--------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+--------------+-------------+--------+---------------+--------------+---------+----------------+--------+-------------+ | 1 | PRIMARY | s | ALL | PRIMARY | NULL | NULL | NULL | 114137 | | | 1 | PRIMARY | t | eq_ref | PRIMARY,lid | PRIMARY | 18 | const,db.s.lid | 1 | Using where | | 1 | PRIMARY | <subquery2> | eq_ref | distinct_key | distinct_key | 4 | func | 1 | | | 2 | MATERIALIZED | l | ALL | string_type | NULL | NULL | NULL | 85039 | Using where | +------+--------------+-------------+--------+---------------+--------------+---------+----------------+--------+-------------+
After adding an appropriate index with
alter table locales_location add index (type, name);
,
these queries run much faster and the entire update only took only 36sec! The explain of such a single query with that index is+------+--------------+-------------+--------+------------------+---------+---------+----------------+------+-----------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+--------------+-------------+--------+------------------+---------+---------+----------------+------+-----------------------+ | 1 | PRIMARY | <subquery2> | ALL | distinct_key | NULL | NULL | NULL | 1 | | | 1 | PRIMARY | s | eq_ref | PRIMARY | PRIMARY | 4 | db.l.sid | 1 | | | 1 | PRIMARY | t | eq_ref | PRIMARY,lid | PRIMARY | 18 | const,db.l.sid | 1 | Using where | | 2 | MATERIALIZED | l | ref | string_type,type | type | 1074 | const,const | 1 | Using index condition | +------+--------------+-------------+--------+------------------+---------+---------+----------------+------+-----------------------+
I think that this is a major issue. An update that takes more that 16min looks like the update process is frozen.
If we combine this with the initially reported issue, the index should be
alter table locales_location add index (type, name, sid);
- Status changed to Needs work
2 months ago 9:43am 1 July 2024 - π¬π§United Kingdom alexpott πͺπΊπ
@mkalkbrenner thanks for the sleuthing.
There's is already an index on
'indexes' => [ 'string_type' => ['sid', 'type'], ],
So I think - looking at the queries on the issue and in the code that we only need to add an index on type and name. The query that requires this index is in \Drupal\locale\StringDatabaseStorage::dbStringSelect(). The other queries in the code all use lid (the primary key) or have sid and type defined which is already indexed.
- Status changed to Needs review
2 months ago 10:02am 1 July 2024 - π¬π§United Kingdom alexpott πͺπΊπ
This issue is going to need us to resolve π Handling update path divergence between 11.x and 10.x Fixed ... as ideally we'd be able to backport this to 10.x
- π¬π§United Kingdom catch
We could avoid blocking this on π Handling update path divergence between 11.x and 10.x Fixed if we backport it to 10.3, given it's in locale module so should benefit everyone with locale module installed, and won't run on sites without it installed, that might be OK but would need a second opinion.
Alternatively we might be OK if we have it in 10.4, 11.0 and 11.1 (and not 10.3) - but only if we don't backport any other database updates to locale module in 10.3 (very unlikely).
- π¬π§United Kingdom alexpott πͺπΊπ
I think given it is an index addition planning to backport everywhere and get it in 11.0.x before 11.0.0 makes sense. Will change the update number to reflect this.
- π¬π§United Kingdom alexpott πͺπΊπ
I've added an MR for 10.x and set the update number to 10101 which is the next available number. This could now be committed to 10.3.x and up as long as we get this done before 11.0.0....
- π¬π§United Kingdom longwave UK
FWIW the impact here is low and to ease the transition between Drupal 10 and 11 I agree that we should backport this to 11.0.
I would suggest setting the update number to 10300 if this is going to land in 10.3 though as that follows the convention we have used so far.
- π¬π§United Kingdom alexpott πͺπΊπ
Changed the update number to @longwave's recommendation.
- Status changed to Needs work
2 months ago 4:06pm 1 July 2024 - π¬π§United Kingdom catch
Two more comments on the MR - one from me, one via @xjm in slack. Other than that looks good though and I think we should backport to 10.3
- π©πͺGermany mkalkbrenner π©πͺ
I tested https://git.drupalcode.org/issue/drupal-3156439/-/tree/3156439-db-table-... and updated from 10.2 to 10.3 again. I can confirm that it increased the speed of the update as expected, 30 seconds instead of more than 16 minutes!
- Status changed to Needs review
2 months ago 8:20am 2 July 2024 - π¬π§United Kingdom alexpott πͺπΊπ
I've fixed two of the MR comments. I'm not convinced about the defensive coding for the update. I don't think we should support custom schema variations on module provided schema. And it erroring is good because in I think it requires human interaction to know what to do... say the index is called type_name and is an index on type, name and sid for example. Or it is sid, type, name as per the example original request.
- Status changed to RTBC
2 months ago 8:40am 3 July 2024 -
longwave β
committed dd402f2f on 10.3.x
Issue #3156439 by alexpott, catch, bonsaipuppy, mkalkbrenner, longwave,...
-
longwave β
committed dd402f2f on 10.3.x
-
longwave β
committed 5f2858c5 on 10.4.x
Issue #3156439 by alexpott, catch, bonsaipuppy, mkalkbrenner, longwave,...
-
longwave β
committed 5f2858c5 on 10.4.x
-
longwave β
committed bb09a543 on 11.0.x
Issue #3156439 by alexpott, catch, bonsaipuppy, mkalkbrenner, longwave,...
-
longwave β
committed bb09a543 on 11.0.x
-
longwave β
committed e81e6f86 on 11.x
Issue #3156439 by alexpott, catch, bonsaipuppy, mkalkbrenner, longwave,...
-
longwave β
committed e81e6f86 on 11.x
- Status changed to Fixed
2 months ago 9:52am 3 July 2024 - π¬π§United Kingdom longwave UK
Thanks, I think this satisfies all comments on the MR now.
Committed and pushed e81e6f869d to 11.x and bb09a54352 to 11.0.x. Thanks!
Committed and pushed 5f2858c540 to 10.4.x and dd402f2fec to 10.3.x. Thanks!
Tagging for 10.3.1 release notes as we don't usually add update hooks to patch releases.
- Status changed to Needs work
2 months ago 11:14pm 3 July 2024 - π³πΏNew Zealand quietone New Zealand
The update test here is failing on PostgreSQL, https://git.drupalcode.org/project/drupal/-/pipelines?page=1&scope=all&r...
- π¬π§United Kingdom alexpott πͺπΊπ
Created a small MR to get the test to pass on Postgres... by just skipping the index inspection shenanigans there.
- π¬π§United Kingdom alexpott πͺπΊπ
alexpott β changed the visibility of the branch 3156439-db-table-localeslocation-10.x to hidden.
- Status changed to Needs review
2 months ago 8:18am 4 July 2024 - π¬π§United Kingdom alexpott πͺπΊπ
alexpott β changed the visibility of the branch 3156439-db-table-localeslocation to hidden.
- Status changed to Fixed
2 months ago 8:28am 4 July 2024 - π¬π§United Kingdom alexpott πͺπΊπ
@quietone opened π Fix index test in LocalesLocationAddIndexUpdateTest::testExistingIndex Needs review so marking this back as fixed.
- π¬π§United Kingdom alexpott πͺπΊπ
alexpott β changed the visibility of the branch 3156439-postgres-followup to hidden.
Automatically closed - issue fixed for 2 weeks with no activity.