MySQL Schema class needs to check for existing indexes for alter queries

This gonna be fun to solve ... because in order to do that we would need the full schema of the table, which is not something we have.

http://dev.mysql.com/doc/refman/5.7/en/show-index.html

return db_query('SHOW INDEX FROM {tablename} WHERE column_name = :new_column_name')->fetch(); or somesuch. If there are results, check Sub_part.

Re-roll it with the SET GLOBAL commands by three db_query and we will see.

Note: the testbot my.cnf is http://cgit.drupalcode.org/drupalci_testbot/tree/containers/database/mys... this.

I do not understand anything. Not only does this fail when we supposedly switch on large index support, it fails just before row format alter, same for jibran. But for me, it fails *after* the alter, on line 706, not on line 703. To recap:

    db_create_table('test_table', $table_spec);
    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 191]);
    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 192]); // bot and jibran env fails here. After those three settings at least but not before.
    db_query('ALTER TABLE {test_table} ROW_FORMAT = COMPACT');
    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 191]);
    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 192]); // it gets here for me but not further. This is the only thing that makes sense in this issue, compact doesn't support large prefixes so this must fail. How on earth does this pass in #2? The only logical explanation would be the table not support utf8mb4 somehow but NodeViewTest::testMultiByteUtf8 has a 4 byte UTF-8 character stored...

Updating description based on 767 not being a limitation but a default.

@chx: I believe above fails because the ROW_FORMAT=COMPACT instead of DYNAMIC.

There are two questions here, really: a) why does #2 pass? b) why does #7 fail before the row format change.

I apologize - I am not quite sure what you mean by #2 and #7 (not a drupal developer). But could this be what you are describing:
http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_inn...

(It now defaults strict, but it won't be in the version CI is using. It makes sense to turn it to strict for CI, since you typically want to catch these errors.)

Oh I meant #2, a patch and #7, another patch , it's the comment numbers of this issue. I apologize for not being clear.

So I have an answer for you. The setting innodb_large_prefix is escalating a warning to an error. Here are the test cases in MySQL-only form, which is a little easier to follow:

# first test (comment #2)

DROP TABLE IF EXISTS test_table;

CREATE TABLE test_table (f1 INT, INDEX(f1)) character set utf8mb4;
ALTER TABLE test_table CHANGE f1 f1 varchar(191);
ALTER TABLE test_table CHANGE f1 f1 varchar(192); # results in 2 warnings

ALTER TABLE test_table ROW_FORMAT = COMPACT; # does nothing, compact is already the default in 5.5 (testcase probably meant to switch this to DYNAMIC?)

ALTER TABLE test_table CHANGE f1 f1 varchar(191);
ALTER TABLE test_table CHANGE f1 f1 varchar(192);  # results in 2 warnings

# second test (comment #7)

SET GLOBAL innodb_large_prefix = ON; # This line changes the warning to an error
SET GLOBAL innodb_file_format=Barracuda;
SET GLOBAL innodb_file_per_table=ON;

DROP TABLE IF EXISTS test_table;

CREATE TABLE test_table (f1 INT, INDEX(f1)) character set utf8mb4;
ALTER TABLE test_table CHANGE f1 f1 varchar(191);
ALTER TABLE test_table CHANGE f1 f1 varchar(192); # results in an error

ALTER TABLE test_table ROW_FORMAT = COMPACT; # does nothing, this is the default for 5.5

ALTER TABLE test_table CHANGE f1 f1 varchar(191);
ALTER TABLE test_table CHANGE f1 f1 varchar(192);  # results in an error

OK, we now understand what is happening. My local environment is MySQL 5.7.

So to recap: innodb_large_prefix got introduced in 5.5. This allows creating longer than 767 byte indexes if all of the following is in place:

1. Barracuda file format
2. Per file storage
3. Dynamic row format

In 5.5 the default for innodb_large_prefix was OFF but in 5.7 it is ON. Soon you won't be able to switch it off at all as it is deprecated. Besides potentially allowing for larger indexes it also causes MySQL to stop truncating indexes with a warning and throw an error instead. This is visible for example in this https://bugs.mysql.com/bug.php?id=68453 bug report. This is the reason the bot doesn't fail in #2 but fails in #7. Note: it should fail and the bot needs to be reconfigured as in the sister issue, we are hiding a warning and if in a future MySQL release when innodb_large_prefix gets removed and hardwired to ON we are SOL.

As for the ROW_FORMAT again the difference is in 5.7 vs 5.5: 5.7 has a new setting called innodb_default_row_format which is DYNAMIC by default while previously the row format defaulted to COMPACT and you needed to change this table by table. This is why bot / jibran fails before the ROW_FORMAT change while my environment goes to the last error as visible in the screenshot attached. It also means that for complete testing this is necessary because people will have both COMPACT and DYNAMIC tables lying around.

And , before I forget: thank you Morgan for your patient guidance, it has been invaluable help to understand this problem.

@chx: one quick comment in response to future changes -
The concept of file formats itself is really just backwards compatibility (the newer being the super-set to the previous, and there are not typically pros/cons of each. See my answer on StackExchange here). I think you should expect both innodb_large_prefix + innodb_file_format to disappear at some point.

+    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 192]);
+    db_query('ALTER TABLE {test_table} ROW_FORMAT = COMPACT');

In fact... the alters after this is not needed because when we switch to COMPACT then the system dies if the index is 192 at this point. So just switch to COMPACT and assert the index is indeed 191.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -696,6 +697,47 @@ function testSchemaChangeField() {
+    db_change_field('test_table', 'f1', 'f1', ['type' => 'varchar', 'length' => 192]);

Doesn't Schema::addIndex() silently set this to be 191 anyway? See Schema::getNormalizedIndexes().

Yes, I mean in the proposed fix, not the current situation -- when altering a field to be a varchar of 191+ characters, we would shorten the index to be 191 characters. So #22 would not apply (the system wouldn't die -- it would just silently shorten the index (or the field length, I don't remember).

Exactly, my comment might have been too terse but I said "and assert the index is indeed 191" . That's what I meant.

Curious if this one is still an issue?