Throw an exception if a schema defines a key that would be over 1000 bytes in MySQL

Also note that the comments needs to say MyISAM incompatible, not MySQL incompatible, MySQL is a somewhat thin layer around its storage engines. You can include really crazy things as MySQL storage engines, like http://blog.mariadb.org/announcing-the-cassandra-storage-engine/ . So it's the storage engine that has limits like this not MySQL. And while Drupal defaults to InnoDB to avoid a complicated discussion on whether a table is best as InnoDB or MyISAM, the latter still has its uses -- having deadlocks is just one of the horrific tolls you pay for the ACID compliance of InnoDB.

In short: yes, enforce 333 because it's the lower common denominator and do not make Drupal MyISAM incompatible.

To be perfectly honest we do not necessarily need to restrict the column length themselves, we could even revert the quickfix I filed for the user data patch -- we could just use the length in the index. Something like array(uid, array(module, 128), array(name, 128)) and have name for example become a TEXT even. Yea that would be better.

Calculating the total length of indexes (taking explicitly specified limits into account but otherwise falling back on the column schema definitions) and throwing an exception makes sense to me.

However, no need to revert the user.data fix, as we still need to limit the module name length throughout Drupal core, since that has additional, other consequences.

making name a TEXT in userdata however would make sense given it comes from a PHP array which had no limitations...

{users_data}.value is a blob already. .name is limited to 128 chars, which should be sufficient for all use-cases. If there is something longer, then that's what the upgrade path is for.

The upgrade path is going to break if the original blob contained two array keys equal at the first 128 characters:

    $data = unserialize($data);
      foreach ($data as $name => $value) {
        $query->values(array(
          'uid' => $uid,
          'name' => $name,

see.

Doesn't this issue violate the general mantra of "Drupal doesn't babysit broken code"?

• #903730-57: Missing drupal_alter() for text formats and filters
• #1279680: watchdog() does not type its array arguments

And also the mantra of "Drupal is database-agnostic" ?

• Database API provides a standard, vendor-agnostic abstraction layer
• #50669: Make database.mysql engine agnostic

Also, these search strings match exactly one page: this one.

• chx "writing a non-mysql compliant database schema should be frown upon"
• chx "all database drivers are equal but some are more equal than others."

Yes, those were quotes only because I said them on IRC.

There's a certain difference between a call that gives you a fatal on some log page which can be fixed by deleting a single line from the database and a Drupal that simply does not install on certain system which is what this issue is about.

Yes we are database agnostic which means that we really to write against the lowest common denominator.

Of course, as I mentioned above, an exception might not be necessary. We might just add something to the MySQL driver to enforce a summary key length of 1000, for example, by enforcing a max 64 character index column length on VARCHAR if the total character number is above 333.

I'd rather object to db-driver specific magic. At least I don't see how a machine can do informed decisions on which parts of an index can be shortened and which cannot. That's something the application developer/DBA has to define based on the business logic.

I actually don't see what's wrong with throwing an exception early and unconditionally. If that means that developers are not able to install or update a module or even entire Drupal at all, and they immediately get to know that they need to fix stuff, that's positive in my book.

Sure I am fine with an exception too. Just wanted to say, we do not need to shorten the DB columns themselves -- we can shorten the index only which IMO is not something we typically did before.

Another one: #1871032: Taxonomy module fails to install on MyISAM due to too long schema index

So here's an initial proposed table index validation implementation.

The validation is only possible when creating a new table, since we need access to the full table definition. Therefore, the validation cannot run when adding new fields, changing fields, or adding individual indexes.

Sorry.

This is not why the patch fails but +class SchemaIndexLengthException extends SchemaException implements DatabaseException { } the implements is not necessary, http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!SchemaExc... already implements DatabaseException . Also, throw new SchemaIndexLengthException(t('Table index @name is too long. Found @length characters (exceeds maximum of @max).', array( i think we format_string in exception, it's not useful to translate those.

Yeah, the actual reason is that the Standard profile fails to install with:

Table index taxonomy_tree is too long. Found 510 characters (exceeds maximum of 333).

:) — which means that we need to fix #1871032: Taxonomy module fails to install on MyISAM due to too long schema index first. I already proposed a fix over there.

Also we need a help page to describe this because there's a different ways to fix index length

1) recompile mysql with bigger limit
2) make a truncated index like #1871032-11: Taxonomy module fails to install on MyISAM due to too long schema index

It could probably use a comment, but yes, this works - the else condition retrieves the trimmed/limited length for the column from the key definition:

+        foreach ($fields as $field) {
           # 'foo'
+          if (is_string($field)) {
...
+          }
           # array('foo', 64)
+          else {
+            $length += $field[1];
+          }

This is just a re-upload of database-schema-1852896-27-do-not-test.patch so that it can be tested now.

I guess we need to check the other databases if there are similar limitations. But I think throwing an exception right there is given for something like this, where we know that it is not going to work for one of the supported backends and there is no way around it.

Well, both SQL Server and Oracle have pretty sophisticated drivers in contrib. I can see that it might not make sense to directly support that in core, but maybe we could at least make this easy to override for them. I'm not familiar enough with the DB system to know if one can override e.g. the \Drupal\Core\Database\Schema class (which the patch above modifies) as a contrib driver.
At the very least we should ping Damien Tournoud and aaaristo about this. It would be sad to see this nice in-depth research go to waste and then have them figure this out on their own when someone files a bug over there.

@tstoeckler: The Schema class can and is extended by by all drivers, create table statements and so on are not really standardized anyway.

Also, this is not about driver-specific validation. That's not really necessary, if it will blow up for the driver that you are using then, well, it will blow up and you can fix it. This is validation for making sure that it works for all supported backends even if you are not using it yourself. And core has a hard enough time to maintain the supported backends (just check how many PostgreSQL 6.x -> 7.x uprade bugs are still open).

Making it a validation that prevents the installation of modules has the advantage over a test that it will also validate all contrib modules, as they are written.

This is validation for making sure that it works for all supported backends even if you are not using it yourself.

Yes, that's how I understood the patch. I was assuming that we don't want to go the route of supporting Oracle and SQL Server directly in core (which would limit us to 260 characters if I'm not mistaken, per @xjm's findings). But if Schema is overridden anyway, those contrib backends can easily adjust Schema::MAX_INDEX_LENGTH that is introduced here and get the full benefit of the validation of this patch. Of course, they wouldn't have the full treatment of all (contrib) modules directly being validated, but still.

Postgresql have only 2 limits:
#1148856: Postgres schema doesn't support keylength on a unique index
and wiki

One limitation is that indexes can not be created on columns longer than about 2,000 characters.

#28: database-schema-1852896-28.patch queued for re-testing.

Re-tested to confirm that it should now correctly fail.

Let's not forget this for updates as well. #1976110: D8 upgrade path: file module; Update #8001 was reported.

#52: database-schema-1852896-52-PASS.patch queued for re-testing.

Here's a re-roll.

My suggestion would be to get this in asap. We can always improve for other database backends, but it's main purpose is to notify us before we break this again and make @patrickd_drupal's life miserable :) That's not fair considering he's given us simplytest.me ;)

A MyISAM test bot would also solve this #697220: Run tests on all supported database servers. Downgrading to normal - I don't consider working around Drupal.org infrastructure a critical task.

The advantage of having this as a core check means that it is detected much earlier and also for contrib and custom modules. If you wait until the testbot breaks, then even for core, it was already committed and you possibly need to think about upgrade changes and so on.

Rerolled patch from #57

Forgot to attach my interdiff for the previous patchfile.

This fixes a couple of bugs in testSchemaMaxIndexSize() in SchemaTest.php.

First, the size of an Int in a MySQL table is 4 bytes, not 8, so the size calculations
were off. I corrected the test by adding an additional int field to the test table
and key.

Also fixed the name of the variable referencing the test table.

Finally, corrected the messages reported by the tests themselves.

The tests all pass for me now, BUT there is now a DatabaseException thrown:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1170 BLOB/TEXT column 'text_field_1' used in key specification without a key length: CREATE TABLE {test_table} ( `text_field_1` TEXT(255) DEFAULT NULL, `text_field_2` TEXT(66) DEFAULT NULL, `numeric_field` DECIMAL DEFAULT NULL, `int_field_1` INT DEFAULT NULL, `int_field_2` INT DEFAULT NULL, PRIMARY KEY (`int_field_1`, `text_field_1`, `text_field_2`, `numeric_field`, `int_field_2`) ) ENGINE = InnoDB DEFAULT CHARACTER SET utf8 COMMENT 'Test schema table.'; 

This appears to be due to MySQL constraints on the primary key.

Argh. I fat-fingered the status, setting back to needs review.

OK, I think I finally have this right. Sorry for the confusion. Comment #66 still applies.

Considering that the primary goal of this issue is to ensure that Drupal 8.x HEAD continues to work on MyISAM I think we should just move this to a test that:

• loads all the module install files
• invokes all the schema to build a schema array
• tests the indexes on these schema to ensure they will work with MyISAM
• if an incompatible index is discovered assert a fail

Changing the type of the two text fields used for keys from text to varchar makes the test pass.

I suppose this raises the question of whether there should be a test for primary keys containing text/blob fields.

New patch with two line change as suggested in #74.

I'd like to mention here that we have upgrade path broken for now - all upgrade tests are failed in myIsam environment
Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 1000 bytes: ALTER TABLE {file_usage} CHANGE `id` `id` VARCHAR(64) NOT NULL DEFAULT '' COMMENT 'The primary key of the object using the file.'; Array ( ) in Drupal\Core\Database\Connection->query() (line 571 of ..../core/lib/Drupal/Core/Database/Connection.php).

A quick hack to allow pass tests attached.

Updated issue summary.

75: database-schema-1852896-75.patch queued for re-testing.

We need to re-look at the key length limit since we no longer support MyISAM (#2275535: [policy, no patch] Drop support for non-transactional database engines (including MyISAM)). InnoDB has a different max length and I'm not sure about postgres or SQLite.

Prefixes can be up to 767 bytes long for InnoDB tables or 3072 bytes if the innodb_large_prefix option is enabled however innodb_large_index was only introduced in 5.5.14.

Sqlite appears to have no limits http://www.sqlite.org/lang_createindex.html

Postgres appears to have no limits: http://www.postgresql.org/docs/9.1/static/sql-createindex.html

Prefixes can be up to 767 bytes long for InnoDB tables or 3072 bytes if the innodb_large_prefix option is enabled however innodb_large_index was only introduced in 5.5.14.

Is there actually a innodb_large_index configuration? I can't seem to find any documentation for it, and I'm hitting the error Specified key was too long; max key length is 1000 bytes when trying to convert tables to utf8mb4 (see #2762599: PDOException while converting cache_block table). It seems the problem is an index on the filter_format table.

[Edit: Note that the problem I was encountering was due to trying to convert a really old MyISAM-ridden database to UTF8mb4; after converting its tables to InnoDB everything worked fine.]

Re rolled. With last few lines not changed.

Re rolled.