MySQL index normalization only works on table create

What other use cases can we think of where this is potentially a problem, is this isolated to yet to be developed contrib modules?

If as far as core is concerned the problem only shows up in onEntityUpdateType() I wonder if we actually need to be normalizing in addIndex() or whether it would be enough to just update onEntityUpdateType() to do $indexes = getNormalizedIndexes($schema);

Otherwise catching the exception with a more helpful error message might be another possible solution.

Hmm OK so doing this in onEntityUpdateType would still be a bit ugly given the index normalization is likely to remain mysql-specific.

In order to do the normalization in addIndex() we'd need to have the full table schema somehow (not just the index specification).

Yes that should work. Still, as you mentioned we may run into the same problem elsewhere, any time contrib/custom/core code were to call addIndex() directly it'd also need to call getNormalizedIndexes().

So it could be good to either mention that in the docblock for addIndex(), or maybe we could deprecate addIndex() and add an addNormalizedIndex() method that takes the full table spec as a parameter, what do you think?

I could go with either... if it's just addIndexes() maybe we could still deprecate addIndex().

Bumping to critical - this means fatal errors on some entity auto-updates, and they're also fatal errors which the current testbot configuration doesn't catch because it has long index support enabled, so they won't necessarily be caught when writing/testing the updates.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -294,19 +294,13 @@ protected function createKeysSql($spec) {
+    $indexes = isset($spec['indexes']) ? $spec['indexes'] : [];

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -422,6 +422,19 @@ public function fieldExists($table, $column) {
+  public function getNormalizedIndexes($spec) {

Sounds like this could us ethe parent method

Just a draft of a patch, anyone feel free to work further on this

We discussed this on the 8.x critical triage call with webchick, xjm, alexbronstein and alexpott and all agreed that it's release blocking - due to fatal errors for straightforward updates that would have worked as table creations, and also the unpredictability between environments.

Adding triaged tag.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -497,6 +497,21 @@ public function addIndex($table, $name, $fields) {
+      throw new SchemaIndexNotInSpecException(t("The index @name doesn't exist in the @table table specification.", array('@table' => $table, '@name' => $name)));
...
+      throw new SchemaObjectDoesNotExistException(t("Cannot add index @name to table @table: table doesn't exist.", array('@table' => $table, '@name' => $name)));
...
+      throw new SchemaObjectExistsException(t("Cannot add index @name to table @table: index already exists.", array('@table' => $table, '@name' => $name)));

Quick note: we don't translate exceptions ... is there a reason why we can't share all the common code between the implementations?

opened #2538814: Do not use t() in exceptions for that.

I guess we could share the exception code, would you suggest a separate method? Something like validateNormalizedIndexes($indexes, $table, $name)

@stefan.r
Well there is no reason to introduce new ones.

Added test & exception

Yes, if we think we can make an exception to the API freeze here, addIndex() does make more sense as a method name.

The worry would be that maintainers forget to upgrade their modules but considering the number of D8 modules is currently manageable maybe we could grep through all existing contrib modules (both now and before D8 release) and personally chase the maintainers? Which contrib modules currently call this method, is it limited to database drivers?

Also if we do change the 3rd parameter we'd need to do some slightly stricter checking of the 3rd argument so we can distinguish between a table spec and a list of index fields and throw a specific exception in case someone still uses $fields instead of $spec.

So to be clear, I think naming it addIndex() is the right thing to do, given that this is what people expect to exist.

The current signature of the patch looks like the following:

add(normalized)Index($table, $name, $fields)

What about doing the following:

addIndex($table, $name, $fields, array $full_table_spec)

This makes it a) really clear how your index looks like while reading the code. This is not really the case IMHO with the current proposed API.
On top of it b) existing calls to that method will break immediately which is IMHO both okay (given we solve a critical) but at the same time makes it really visible, so contrib/custom code can be adapted. With that signature I would suggest merging in the index fields into the full table spec, so you don't need to specify it twice.

Curious why we can't take into account #23 beside laziness :)

sounds fine to me, do we have an idea about which contrib modules call addIndex() so we know what exactly we're breaking?

sounds fine to me, do we have an idea about which contrib modules call addIndex() so we know what exactly we're breaking?

I bet the only common usecase are in hook_update_N() functions.

Cool, let me give that a quick try.

1. Expanded the test coverage a bit for the exceptions
2. Refactored the exception throwing logic into its own method. There I also removed the t() function from the exception itself. This really is just old code we are dealing with.
3. Changed the API as written above
4. Once done 3) I realized that the new exception will never be thrown on a real life, because the schema::addIndex implementations will take care of it, so I removed that again.

Let's fix the failures.

Updated the issue summary

Nice work, patch looks good! This will probably need a change record?

Here is one.

Updated the IS

+1 Thank you stefan.r

So I think we just need someone who RTBCs this patch.

Patch looks OK, one small thing:

+++ b/core/includes/database.inc
@@ -654,9 +654,11 @@ function db_drop_unique_key($table, $name) {
+ * @param array $spec
+ *   A table specification.
...
+function db_add_index($table, $name, $fields, array $spec) {

We don't normally use abbreviated variable names?

If we decide to unabbreviate that we may want to update the following too as they use the abbreviated name $spec as well:

Schema::createKeysSql(),
Schema::createFieldSql(),
Schema::addField(),
Schema::getNormalizedIndexes()
Schema::changeField()
db_add_field()
db_change_field()

If we decide to unabbreviate that we may want to update the following too as they use the abbreviated name $spec as well:

Yeah this is why I think this is fine. Spec is now also not a horrible name for a specification.

OK, +1 from me.

SchemaTest passes in both mysql and pgsql. But pgsql is broken cause...

  protected function _createKeys($table, $new_keys) {
    if (isset($new_keys['primary key'])) {
      $this->addPrimaryKey($table, $new_keys['primary key']);
    }
    if (isset($new_keys['unique keys'])) {
      foreach ($new_keys['unique keys'] as $name => $fields) {
        $this->addUniqueKey($table, $name, $fields);
      }
    }
    if (isset($new_keys['indexes'])) {
      foreach ($new_keys['indexes'] as $name => $fields) {
        $this->addIndex($table, $name, $fields);
      }
    }
  }

in core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php the call to addIndex is broken.

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -413,13 +413,55 @@ public function fieldExists($table, $column) {
+  /**
+   * Validates whether the index is valid.
+   *
+   * This method ensures that the table exists and that there is no index with
+   * the same name yet.
+   *
+   * @param string $table
+   *   The name of the table the index will be added to.
+   * @param string $name
+   *   The name of the index.
+   *
+   * @throws \Drupal\Core\Database\SchemaObjectDoesNotExistException
+   *   If the specified table doesn't exist.
+   * @throws \Drupal\Core\Database\SchemaObjectExistsException
+   *   If the specified table already has an index by that name.
+   */
+  protected function validateIndex($table, $name) {
+    if (!$this->tableExists($table)) {
+      throw new SchemaObjectDoesNotExistException("Cannot add index '$name' to table '$table': table doesn't exist.");
+    }
+    if ($this->indexExists($table, $name)) {
+      throw new SchemaObjectExistsException("Cannot add index '$name' to table '$table': index already exists.");
+    }
+  }

Adding this unnecessary for the fix. I think we don't need to do this. Less change is nice.

There we go.

Let's add some comment.

Ups

#56 is addressed so back to RTBC.

I've asked @Crell to have a look as sub system maintainer.

Thanks for the tag, Alex!

1. +++ b/core/includes/database.inc
   @@ -654,9 +654,11 @@ function db_drop_unique_key($table, $name) {
   + * @param array $spec
   + *   A table specification.
   

   Specification of what table? $table's specfication? That's unclear.

2. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -413,13 +413,29 @@ public function fieldExists($table, $column) {
   +   * @param array $spec
   +   *   A table specification, which is used in order to be able to ensure that
   +   *   the index length is not too long.
       *
       * @throws \Drupal\Core\Database\SchemaObjectDoesNotExistException
       *   If the specified table doesn't exist.
       * @throws \Drupal\Core\Database\SchemaObjectExistsException
       *   If the specified table already has an index by that name.
       */
   -  abstract public function addIndex($table, $name, $fields);
   +  abstract public function addIndex($table, $name, $fields, array $spec);
   

   I talked through this in IRC with dawehner and catch. I am very skeptical about the DX of requiring the full table definition to be passed in each time, as there's no common place to get that information. It requires out-of-band data, retrieved from... somewhere context sensitive. Could be hook_schema(), could be derived from somewhere like Entity API or cache API if it's a dynamic table... you have to "just know" where to get this big array from. That's the terrible DX we've been trying to move away from.

   Unfortunately, the only alternative we could come up with is to add schema introspection to the DB API, so that it could return a table spec of a table in the database already. Then the schema object could just look at the table itself and know what it needs to do. That, however, is a not currently a feature of the Database API and is a heck of a lot of work to add, in part "because MySQL". *sigh* That is out of scope at this time.

   So if we're going to move forward here, we should:

   1) Do a much better job of documenting where to get the value of $spec. Even if that's just "depending on the use case it could be from X, or Y, or Z". Give developers some sort of indication of how to use this method.

   2) Leave a @todo on the method that if we ever do manage to add schema introspection later then the $spec array can be removed entirely.

   3) catch pointed out in IRC that $spec technically doesn't need to be the full table, just the fields that are being used by the index. Any others are irrelevant. I am torn on if we should say that or not; it could make it easier to use if someone is adding a field and then an index on it, since they're almost guaranteed to have the spec for that field handy already, but it could also add complications as you're passing in a table definition that is not a full table definition but works anyway, and unstructured inputs lead to fun-to-find bugs. Open to thoughts on this point.

1. +++ b/core/includes/database.inc
   @@ -907,16 +905,22 @@
    function db_add_index($table, $name, $fields) {
   -  return Database::getConnection()->schema()->addIndex($table, $name, $fields);
   +  return Database::getConnection()->schema()->addIndex($table, $name, $fields, $spec);
    }
   

   Mh, that can't work ... you need to add $spect to db_add_index as well

2. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -414,13 +414,48 @@
   +   *   SqlContentEntityStorageSchema::getDedicatedTableSchema() and ¶
   

   empty whitespace

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -413,13 +413,64 @@ public function fieldExists($table, $column) {
+   *   To prevent human error, it is recommended to pass in the complete table
+   *   specification. However, in the edge case of the complete table specification
+   *   not being available, we can pass in a partial definition containing only
+   *   the fields that apply to the index:

+1

1. +++ b/core/includes/database.inc
   @@ -654,11 +654,9 @@
   - * @param array $spec
   - *   A table specification.
     */
   -function db_add_index($table, $name, $fields, array $spec) {
   -  return Database::getConnection()->schema()->addIndex($table, $name, $fields, $spec);
   +function db_add_index($table, $name, $fields) {
   +  return Database::getConnection()->schema()->addIndex($table, $name, $fields);
   

   Why undo this change? db_add_index() is deprecated but it still needs to work.

2. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -414,13 +414,48 @@
   +   *   handler listed in the entity class definition (see
   +   *   SqlContentEntityStorageSchema::getDedicatedTableSchema() and ¶
   +   *   SqlContentEntityStorageSchema::getSharedTableFieldSchema().
   

   Nit: Stray whitespace at the end of line 423.

The revised comment looks good to me otherwise.

Hmm that was an oversight, $spec was in the docblock and inside the function but not in the parameters.

with the parameter restored

Thanks, stefan.r! Back up to RTBC for Alex.

Removing the assignment

This introduces SQLite regressions. Drupal\system\Tests\Entity\FieldSqlStorageTest fails on SQLite with an exception after applying this patch.

We can just make less change. Also I don't think we need to pollute the base Schema namespace with getNormalisedIndexes until we have a use case.

Seems to be failing less on SQLite :) More on PostreSQL :(

@Gábor Hojtsy the problem is the postgres and sqlite Drupalci runs are not stable (or passing) - so manual testing is required :(

Local test runs for me:

SQLite

Drupal\field\Tests\Number\NumberFieldTest fails on HEAD and PATCH
Drupal\image\Tests\ImageEffectsTest passes on HEAD and PATCH
Drupal\page_cache\Tests\PageCacheTest passes on HEAD and PATCH
Drupal\toolbar\Tests\ToolbarAdminMenuTest passes on HEAD and PATCH
Drupal\views_ui\Tests\NewViewConfigSchemaTest passes on HEAD and PATCH

No regression for SQLite

PostgreSQL

@todo

Just from a manual review the current fails seem unlikely to be caused by this patch.

As apparently drupalci for pgsql/sqlite is not stable, could anyone run the patch vs HEAD on pgsql/sqlite locally?

Postgres

The suspicious tests are:

• Drupal\node\Tests\Views\RevisionRelationshipsTest
• Drupal\views\Tests\Handler\FieldFieldTest

All the rest are DrupalCI set issues due to the clear down of a previous test or file system

Drupal\node\Tests\Views\RevisionRelationshipsTest fails on HEAD and PATCH
Drupal\views\Tests\Handler\FieldFieldTest fails on HEAD and PATCH

No regression for Postgres

@alexpott, Yep, both tests fail since some days and are not related to this issue. (as seen on PG-Bot) I think it's just another missing order by problem. So no showstopper for this issue :)

interdiff in #76 looks good and no regressions, so +1 from me

Created:

• #2542124: Drupal\views\Tests\Handler\FieldFieldTest fails on Postgres
• #2542128: Drupal\node\Tests\Views\RevisionRelationshipsTest fails on Postgres
• #2542132: Drupal\field\Tests\Number\NumberFieldTest fails on SQLite

None of these issues are related to this issue.

So the patch is green again.

Doing less in order to fix an issue is often a great idea

Updated the change notice draft significantly to make it easier to understand: https://www.drupal.org/node/2540310/revisions/view/8715856/8725758

Updating issue summary too.

+1, thanks guys!

We have to pass along the fields schema information to addIndex because the MySQL Driver needs the information to perform normalization on long indexes.

And the interdiff ...

Sorry uploaded the wrong patch. Ignore #88.

Good catch!

For #90

BTW the fails in #90 on DrupalCI are to be expected:

Drupal\Core\Installer\Exception\InstallerException: PHP: 5.4.39

Your PHP installation is too old. Drupal requires at least PHP 5.5.9. in install_display_requirements() (line 2292 of /var/www/html/core/includes/install.core.inc). install_display_requirements(Array, Array)

Trying to write some unit test for #90 atm.

Here are some tests. Found another bug introduce my this patch in the entity auto updater.

Drupal\system\Tests\Entity\EntityDefinitionUpdateTest still passes on both pgsql and sqlite.

Let's throw an exception if we don;t have the field spec.

Wrote a unit test in the meantime.

My patch lacked of #96

Throwing the exception found another bug.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -323,6 +327,9 @@ protected function getNormalizedIndexes($spec) {
+          throw new SchemaException("MySQL needs the '$field_name' field specification in order to normalize the '$index_name' index");

We've usually been using sprintf() in these cases. I looked at the string and tried to figure out if we were going to interpolate that variable or not, and got very confused. :-)

I'm otherwise satisfied with this patch. Queuing for SQLite and Pgsql just to be on the safe side.

I looked at the string and tried to figure out if we were going to interpolate that variable or not, and got very confused. :-)

We have no standards on how to concatenate variables in an exception message. sprintf() is totally unnecessary, PHP is very good constructing strings from variables and hard coded strings - let's use the language features. An added bonus is that it reads way better - you actually have to think less. Especially when the number of variables increases.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -323,6 +327,9 @@ protected function getNormalizedIndexes($spec) {
+        else {
+          throw new SchemaException("MySQL needs the '$field_name' field specification in order to normalize the '$index_name' index");
+        }

I'm curious here, why do we not use an assertion? On runtime vs. development time this will never be different. Either code passes it along or not. I still try to wrap my head around the right usecases of assertions though

So I also think that could be an assertion but seems like good material for a follow-up.

Committed/pushed to 8.0.x, thanks!

follow-up about the assertion at #2545312: Turn the SchemaException in mysql\Schema::getNormalizedIndexes() into an assertion

Published change record at https://www.drupal.org/node/2540310

Oof, providing the full spec is likely going to be painful in contrib module update hooks, but I understand why.