Add indexes to important fields

Same with the coupon code and payment remote ID. We need to look at our storage classes, see what we're loading by, ensure indexes.

Reviewing

Reviewing \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema

Indexes are set if the field schema has indexes.

    if (!empty($field_schema['indexes'])) {
      $schema['indexes'] = $this->getFieldIndexes($field_name, $field_schema, $column_mapping);
    }

Currently, we have indexes on the product_id, type, and UID. That is because \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::schema contains

    $schema = [
      'columns' => $columns,
      'indexes' => [
        'target_id' => ['target_id'],
      ],
    ];

In \Drupal\Core\Field\Plugin\Field\FieldType\StringItem::schema there are no index definitions.

I believe the way we work around this is to implement a new storage_schema handler, like \Drupal\node\NodeStorageSchema. This allows us to arbitrarily added index. See the following example

  /**
   * {@inheritdoc}
   */
  protected function getEntitySchema(ContentEntityTypeInterface $entity_type, $reset = FALSE) {
    $schema = parent::getEntitySchema($entity_type, $reset);

    $schema['node_field_data']['indexes'] += [
      'node__frontpage' => ['promote', 'status', 'sticky', 'created'],
      'node__title_type' => ['title', ['type', 4]],
    ];

    return $schema;
  }

Initial patch. No idea what kind of upgrade path we need to do.

PR: https://github.com/drupalcommerce/commerce/pull/794

Picking this up, again, finally!

Rerolled against HEAD, need to test the upgrade still.

Split the updates into each module, as the schema change wasn't being run due to clearing entity definitions.

EDIT: Updates are not applying, properly. I am trying to to determine why \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityStorageSchemaChanges always returns false.

Okay! This fixes our problem. There is a limitation in core where the storage schema handler exits early and does not generate indexes unless there is a change in ability to translate, revisions handling, or a field column change.

Looks promising, and on testing. Going to do a full check on our enormous database tomorrow, where we already have an index on sku. Also we limited sku to 32 characters, but that should probably not be an issue.

eiriksm thanks so much! I know you've got quite the database which means it will take some time :) but I want to make sure the upgrade path doesn't break on your site.

Patch looks good to me.

+    // @todo open issue against Drupal core
+    // ::onEntityTypeUpdate will only populate new indexes if the entity has
+    // a change on being translatable, revisionable, or a field change.

Let's open that issue before we commit the patch :)

EDIT: I tried to investigate the issue myself, but wasn't sure what's going on.

Assuming that this is FALSE:

    if (!$this->requiresEntityStorageSchemaChanges($entity_type, $original)) {
      return;
    }

but the method has:

   return
      $this->hasSharedTableStructureChange($entity_type, $original) ||
      // Detect changes in key or index definitions.
      $this->getEntitySchemaData($entity_type, $this->getEntitySchema($entity_type, TRUE)) != $this->loadEntitySchemaData($original);

and that || should allow for indexes to change even if the shared table structure didn't, no?

Hey. We have a problem running this update. It is of course mostly our own fault, since we did some manual adjustments to the tables.

Here is the error message:

[notice] Update started: commerce_product_update_8205
 [error]  Exception thrown while performing a schema update. SQLSTATE[HY000]: General error: 1089 Incorrect prefix key; the used key part isn't a string, the used length is longer than the key part, or the storage engine doesn't support unique prefix keys: ALTER TABLE {commerce_product_variation_field_data} ADD INDEX `commerce_product_variation_field__sku` (`sku`(191)); Array
(
)
 
 [error]  Update failed: commerce_product_update_8205 
 [error]  Update aborted by: commerce_product_update_8205 
 [error]  Finished performing updates. 

And here is the table (although crashed a couple of times in an update):

CREATE TABLE `commerce_product_variation_field_data` (
  `variation_id` int(10) unsigned NOT NULL,
  `type` varchar(32) CHARACTER SET ascii NOT NULL COMMENT 'The ID of the target entity.',
  `langcode` varchar(12) CHARACTER SET ascii NOT NULL,
  `uid` int(10) unsigned DEFAULT NULL COMMENT 'The ID of the target entity.',
  `product_id` int(10) unsigned DEFAULT NULL COMMENT 'The ID of the target entity.',
  `sku` varchar(32) DEFAULT NULL,
  `title` varchar(255) DEFAULT NULL,
  `price__number` decimal(19,6) DEFAULT NULL COMMENT 'The number.',
  `price__currency_code` varchar(3) DEFAULT NULL COMMENT 'The currency code.',
  `status` tinyint(4) NOT NULL,
  `created` int(11) DEFAULT NULL,
  `changed` int(11) DEFAULT NULL,
  `default_langcode` tinyint(4) NOT NULL,
  `list_price__number` decimal(19,6) DEFAULT NULL COMMENT 'The number.',
  `list_price__currency_code` varchar(3) DEFAULT NULL COMMENT 'The currency code.',
  PRIMARY KEY (`variation_id`,`langcode`),
  KEY `commerce_product_variation__id__default_langcode__langcode` (`variation_id`,`default_langcode`,`langcode`),
  KEY `commerce_product_variation__fc57e9c936` (`type`),
  KEY `commerce_product_variation_field__uid__target_id` (`uid`),
  KEY `commerce_product_variation__cef18578b5` (`product_id`),
  KEY `sku` (`sku`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COMMENT='The data table for commerce_product_variation entities.';

It might very well be because we did not alter the schema after we changed the length of the sku field though, so testing that now.

Can confirm it works if I alter the schema definition to also be correct.

Of course now I have two indexes on SKU, but that seems like something one can manage :)

Following up to #16

\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::requiresEntityStorageSchemaChanges

  /**
   * {@inheritdoc}
   */
  public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
    return
      $this->hasSharedTableStructureChange($entity_type, $original) ||
      // Detect changes in key or index definitions.
      $this->getEntitySchemaData($entity_type, $this->getEntitySchema($entity_type, TRUE)) != $this->loadEntitySchemaData($original);
  }

The first check hasSharedTableStructureChange wil alwas return false, since we did not change any of the entities in regards to the ability to be revisionable, translatable, or change their table names.

The second check fails because

• \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getEntitySchema does not include indexes. It does not add indexes in processDataTable or processBaseTable
• Entities only have indexes added if they implement EntityPublishedInterface, see $schema[$published_field_table]['indexes'][$this->getEntityIndexName($entity_type, $key)] = $columns;
• Then getEntitySchemaData falls short, as well. It ends up invoking \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getFieldSchemaData. This only concerns itself with indexes defined on the field type itself, which is why entity references have indexes properly.

So, unless these fields were their custom field types with specified indexes, there is no way for Drupal core to detect additional indices not defined within the field type.

Linking to the related core issue we will comment in the code doc. I just downloaded my IRCCloud logs of the Drupal Slack to see if I can find my conversation with berdir.

EDIT Found it!

[2018-09-14 05:02:09] <mglaman> If someone wants to add indexes to an entity's schema definition, does one need to manually apply the indexes? \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::updateEntityType doesn't seem to do the trick. 
[2018-09-14 05:04:46] <berdir> @mglaman I think you're looking for \Drupal\node\NodeStorageSchema::getEntitySchema :slightly_smiling_face: 
[2018-09-14 05:05:26] <berdir> and in an update function, you need to set that on the entity definition before calling that 
[2018-09-14 05:05:26] <mglaman> We have one, which adds indexes, but no idea how to run them in an update hook 
[2018-09-14 05:05:57] <berdir> @mglaman see paragraphs_update_8008() for an example 
[2018-09-14 05:06:05] <mglaman> @berdir thank you so much! 

[2018-09-14 10:11:02] <mglaman> @berdir for the Paragraphs update.. the index was confirmed after updates? I'm having issues with Commerce. \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate runs `requiresEntityStorageSchemaChanges`. That seems to always return false unless it becomes revisionable, translatable, or has a column added/remove/renamed 
[2018-09-14 10:11:59] <berdir> @mglaman I think that did a different schema change. yes because indexes don't require schema changes, that should then be done anyway somewhere, checking 
[2018-09-14 10:12:28] <mglaman> @berdir thanks, I've been wracking my brain 
[2018-09-14 10:13:33] <berdir> @mglaman I see,  \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::onEntityTypeUpdate() returns early, but then the index stuff is further down :confused: 
[2018-09-14 10:13:43] <mglaman> Yeah :disappointed: and I have no idea 
[2018-09-14 10:13:54] <berdir> @amateescu ^ 

[2018-09-14 10:16:16] <berdir> @mglaman hm, but it should work IMHO. requiresEntityStorageSchemaChanges() does compare the full entity schema 
[2018-09-14 10:16:34] <mglaman> @berdir when I did an xdebug it was only the primary key, for some reason 
[2018-09-14 10:17:43] <berdir> @mglaman yeah, not sure I fully understand but there is something about removing indexes and unique keys in getEntitySchemaData(), that's.. unfortunate :confused: 
[2018-09-14 10:18:15] <berdir> @mglaman yeah, I'd just add them manually then. 
[2018-09-14 10:18:29] <berdir> and maybe open a bug report about this :slightly_smiling_face: 
[2018-09-14 10:18:29] <mglaman> @berdir :smile: i'm giving it a try 

Rerolled against HEAD and includes link to the core issue.

We've been seeing various database-related performance issues and this patch addresses a specific one we have seen caused by the lack of an index on commerce_order.order_number.

I've reviewed the patch and the indexes included are applied correctly.

Moving back to status Needs Work as we are seeing 'Mismatched entity and/or field definitions' warnings after applying the patch and running updates:

The following changes were detected in the entity type and field definitions.
Order
The Order number field needs to be updated.
Payment
The Remote ID field needs to be updated.
Payment method
The Remote ID field needs to be updated.
Product variation
The SKU field needs to be updated.
Coupon
The Coupon code field needs to be updated.

Updated patch

Looks good, I'll get this committed. I trust @eiriksm has put it through the paces for their fairly large site :)

🥳 Committed! Yay for performance.

@mglaman, we ran into this when testing out some stuff, thought you should know, maybe an update hook is needed?

Here's the docs on the adding index in your update hook:
https://www.drupal.org/docs/drupal-apis/update-api/updating-database-sch...

Ah sorry, should have searched #3005447: Adding an index to an entity's schema is not detected as a change in onEntityTypeUpdate is addressing this I'll create a follow-up

+++ b/src/CommerceContentEntityStorageSchema.php
@@ -0,0 +1,51 @@
+   */
+  public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
+    parent::onEntityTypeUpdate($entity_type, $original);
+
+    // ::onEntityTypeUpdate will only populate new indexes if the entity has
+    // a change on being translatable, revisionable, or a field change.
+    // @see https://www.drupal.org/project/drupal/issues/3005447
+    $entity_schema = $this->getEntitySchema($entity_type, TRUE);
+    $schema_table = $entity_type->getDataTable() ?: $entity_type->getBaseTable();
+    $schema_indexes = $entity_schema[$schema_table]['indexes'];
+    foreach ($schema_indexes as $index_name => $index_fields) {
+      if (!$this->database->schema()->indexExists($schema_table, $index_name)) {
+        $this->database->schema()
+          ->addIndex($schema_table, $index_name, $index_fields, $entity_schema[$schema_table]);
+      }
+    }
+  }
+

This doesn't seem right. If you add an index on a field, then the correct thing to do is call updateFieldStorageSchema() and that should add the index automatically. Don't just update the entity type.

After updating Drupal commerce I get below mention unexpected error.

Drupal\Core\Field\FieldException: Attempt to create a configurable field of non-configurable field storage variations. in Drupal\field\Entity\FieldConfig->getFieldStorageDefinition() (line 315 of /code/core/modules/field/src/Entity/FieldConfig.php)
#0 /code/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(261): Drupal\field\Entity\FieldConfig->getFieldStorageDefinition()
#1 /code/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(154): Drupal\jsonapi\ResourceType\ResourceTypeRepository->getFields(Array, Object(Drupal\Core\Entity\ContentEntityType), 'default')
#2 /code/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(125): Drupal\jsonapi\ResourceType\ResourceTypeRepository->createResourceType(Object(Drupal\Core\Entity\ContentEntityType), 'default')
#3 [internal function]: Drupal\jsonapi\ResourceType\ResourceTypeRepository->Drupal\jsonapi\ResourceType\{closure}(Array, 'default')
#4 /code/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(124): array_reduce(Array, Object(Closure), Array)
#5 /code/core/modules/jsonapi/src/Routing/Routes.php(115): Drupal\jsonapi\ResourceType\ResourceTypeRepository->all()
#6 [internal function]: Drupal\jsonapi\Routing\Routes->routes()
#7 /code/core/lib/Drupal/Core/Routing/RouteBuilder.php(146): call_user_func(Array)
#8 /code/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php(83): Drupal\Core\Routing\RouteBuilder->rebuild()
#9 /code/core/includes/common.inc(1090): Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()
#10 /code/core/modules/system/src/Controller/DbUpdateController.php(652): drupal_flush_all_caches()
#11 /code/core/includes/batch.inc(456): Drupal\system\Controller\DbUpdateController::batchFinished(true, Array, Array, '1 sec')
#12 /code/core/includes/batch.inc(98): _batch_finished()
#13 /code/core/modules/system/src/Controller/DbUpdateController.php(185): _batch_page(Object(Symfony\Component\HttpFoundation\Request))
#14 [internal function]: Drupal\system\Controller\DbUpdateController->handle('start', Object(Symfony\Component\HttpFoundation\Request))
#15 /code/core/lib/Drupal/Core/Update/UpdateKernel.php(115): call_user_func_array(Array, Array)
#16 /code/core/lib/Drupal/Core/Update/UpdateKernel.php(76): Drupal\Core\Update\UpdateKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request))
#17 /code/update.php(28): Drupal\Core\Update\UpdateKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#18 {main}

When I run update.php there were 11 updates. Image attached Drupal-database-update.png