Add an index on locales_location on type and name

Sounds reasonable. Moving to the right version and since there is indeed a trade off here, I'm going to mark this as a feature and not a bug.

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);

@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.

This issue is going to need us to resolve #3108658: Handling update path divergence between 11.x and 10.x... as ideally we'd be able to backport this to 10.x

We could avoid blocking this on #3108658: Handling update path divergence between 11.x and 10.x 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).

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.

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....

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.

Changed the update number to @longwave's recommendation.

One comment on the MR.

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

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!

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.

This looks great now, RTBC for me.

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.

The update test here is failing on PostgreSQL, https://git.drupalcode.org/project/drupal/-/pipelines?page=1&scope=all&r...

Created a small MR to get the test to pass on Postgres... by just skipping the index inspection shenanigans there.

@quietone opened #3458922: Fix index test in LocalesLocationAddIndexUpdateTest::testExistingIndex so marking this back as fixed.

Amending attribution.