Remove 'all' index from {key_value_expire} - serves no purpose and negatively impacts performance

Setting to major, if expire is not in the PK, that is a problem for the query parser IIRC.

To clarify:

- Do we have any key on 'expire' right now?

Overall: +1 to the approach and +1 to getting it in before supporting upgrade path as that could get hairy on production.

Best to delete the table and have key-value-expire recreate it automatically (except its installed by system module still ... so that won't work ... ).

MariaDB [d8]> SHOW INDEXES FROM key_value_expire;
+------------------+------------+----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| Table            | Non_unique | Key_name | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | Index_comment |
+------------------+------------+----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
| key_value_expire |          0 | PRIMARY  |            1 | collection  | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
| key_value_expire |          0 | PRIMARY  |            2 | name        | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
| key_value_expire |          1 | all      |            1 | name        | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
| key_value_expire |          1 | all      |            2 | collection  | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
| key_value_expire |          1 | all      |            3 | expire      | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
| key_value_expire |          1 | expire   |            1 | expire      | A         |           5 |     NULL | NULL   |      | BTREE      |         |               |
+------------------+------------+----------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+---------------+
6 rows in set (0.00 sec)

@berdir, would you mind seeing what happens if you drop the 'all' key? We don't have any queries that need that as far as I can see.

Best to delete the table and have key-value-expire recreate it automatically (except its installed by system module still ... so that won't work ... ).

So I'd be up for fixing that properly, but also the update could just delete and recreate manually for now.

I am triaging Major issues at New Orleans. Nothing has changed about this situation. This is a major performance issue at the database level. I've tagged it on the assumption that this would be a releaseable change. It seems that way based on the conversation.

Thanks @cilefen for helping triage majors at the sprint. Updating issue credit.

Discussed this with @xjm, @alexpott, @webchick and @cottser and agreed this is definitely major due to the severe scalability issue.

This is still valid as far as I can see.

In #2510438-2: Remove 'all' index from {key_value_expire} - serves no purpose and negatively impacts performance I suggested dropping the 'all' key - this would just need system_schema() updating and a straightforward update hook to drop the index. IMO this could still be backported to 8.9.x

We could then open a follow-up to discuss changing the primary key altogether. #2664322: key_value table is only used by a core service but it depends on system install would simplify the update there since we could simply drop the existing table in the update. That kind of change I think we'd need to do only in a minor release, so 9.1.x only.

Working on this :)

+++ b/core/modules/system/tests/src/Functional/Update/DropIndexAllOnKeyValueExpireTableUpdateTest.php
@@ -0,0 +1,35 @@
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../fixtures/update/drupal-8.8.0.bare.standard.php.gz',

Could be dirname(__DIR__, 3) . '/fixtures/update/drupal-8.8.0.bare.standard.php.gz',

Sorry, the patch is empty, Thanks @johnwebdev for the ping!

Re-titling.

Just on the original suggestion here as opposed to the current patch:

@DamZ suggested to change the primary key to (expire, collection, name).

If we did this, we'd have no database-level constraint preventing duplicates for collection, name - so assuming we want to keep that, we'd need to make a new unique index on collection, name. Not sure that would necessarily help with the deletion issue unless it's specifically the primary key causing the problem here.

Based on #24

Given #24 I don't think we need the follow-up here for the primary key change at all.

Needed reroll due to #2664322: key_value table is only used by a core service but it depends on system install.

+++ b/core/modules/system/system.install
@@ -1447,3 +1447,11 @@ function system_update_8901() {
+  $schema->dropIndex('key_value_expire', 'all');

Could we test first for the existence of the table. The function dropIndex() does not like it when you try to drop an index from a non-existing table.

Sure. Test added.

The removed index is not used.
There update function added to the patch for existing sites.
There is testing added to the update function.
For me it is RTBC.

+++ b/core/modules/system/tests/src/Functional/Update/DropIndexAllOnKeyValueExpireTableUpdateTest.php
@@ -0,0 +1,35 @@
+ *
+ * @group Update
+ * @see system_update_8902
+ */

This should be system_update_9201().

However, I'm not sure this needs to be a hook_update_N() since it's only dropping an index rather than changing the way data is scored in the schema. If we made it a post update, it would also help us avoid running into #3108658: [policy] Handling update path divergence between 11.x and 10.x.

Updated as per comment #33.

+++ b/core/modules/system/system.install
@@ -1447,3 +1447,13 @@ function system_update_8901() {
+function system_update_9201() {

This function should be changed to a hook_post_update_NAME() function as requested by @catch in comment #33.

Updated patch in #34 addressing #35. Thanks!

The link given by catch in #33 suggests changes that still need to be implemented here.

The update function has been changed to post update function and that was what @catch wanted in comment #33.
Therefore back to RTBC.

@quietone: If I am missing something, then please explain what.

@daffie, thanks for asking. I reread the issue referred to and learned that I misread the Proposed Resolution, substituting hook_post_update for hook_update which is wrong. So, yes, the changes requested in #33 are complete now.

+++ b/core/modules/system/system.install
@@ -1447,3 +1447,13 @@ function system_update_8901() {
+ */
+function system_post_update_9201() {
+  $schema = \Drupal::database()->schema();

This should be moved to system.post_update.php and also named something like 'system_post_update_remove_key_value_expire_all_index()' - since there's no numbering of post updates.

Committed 1fb62cc and pushed to 9.2.x. Thanks!