Commit 79ab2a90a52369c5880eb96539560930c6e44eb7 breaks SQL index due to improper applying of #6412

[discordier]

As reported by user kozi on IRC the upgrade from 3.1.5 to 3.2.0 causes an SQL error in MySQL 5.6:

<kozi> ALTER TABLE `tl_style` CHANGE `selector` `selector` varchar(1022) NOT NULL default '';
<kozi> Fatal error: Uncaught exception Exception with message Query error: Specified key was too long; max key length is 1000 bytes (ALTER TABLE `tl_style` CHANGE `selector` `selector` varchar(1022) NOT NULL default '';) thrown in system/modules/core/library/Contao/Database/Statement.php on line 282

The length change in commit 79ab2a9 exceeds the maximum limit of an MySQL index in MySQL 5.6

The problem here is, that the PR #6412 has been incorrectly applied.
If you compare it to the original authors submission (25e6d97) the column type MUST be TEXT and not VARCHAR(1022).
Simply merging the PR instead of pre-commit-editing it would have prevented this issue.

With innoDB you can circumvent this behaiour via setting innodb_large_prefix to ON but with MyISAM you appear to be lost.

Reference:
http://dev.mysql.com/doc/refman/5.6/en/innodb-parameters.html#sysvar_innodb_large_prefix
http://dba.stackexchange.com/questions/49913/specified-key-was-too-long-max-key-length-is-1000-bytes-in-mysql-5-6

So in General, long indexes appear to be generally a bad idea in MySQL, we might want to change that maximum length to some lower value again.

[BugBuster1701]

and then there must also be changed here:
https://github.com/contao/core/blob/master/contao/install.php#L1062

[discordier]

Maybe @ralfhartmann can provide a little more information, as he is the author of the original PR.

[leofeyer]

@backbone87 has confirmed that varchar(1022) is a valid length supported by MySQL 5.0+. I did not have any problems neither on my local host nor on any of our servers.

[aschempp]

Maybe the field length is valid, but the key length is not? Why should there be a key on tl_style.selector? Maybe a custom extension?

[BugBuster1701]

@backbone87 has confirmed that varchar(1022) is a valid length supported by MySQL 5.0+.

only on 64 Bit Systems, can it be?

[discordier]

@aschempp The key exists because of

core/system/modules/core/dca/tl_style.php

Line 57 in 79ab2a9

'selector' => 'index'

. therefore nope, no custom extension.

@leofeyer The field size is in fact valid but the key length must not be exceed 1000 bytes for VARCHAR() in certain circumstances.

Interesting fact: on my system (MariaDB 10.0.5 64bit) the index get's cut down to:

KEY `selector` (`selector`(333))

Even when trying to create it as:

ALTER TABLE  `contao3`.`tl_style` ADD INDEX ( `selector` ( 1022 ) ) COMMENT  '';

The math behind (MariaDB appears to be pessimistic and assume the worst-case scenario of every character being a 3 bytes UTF-8 character):

3 * 333 == 999
3 * 334 == 1002

It never exceeds 333 bytes, but maybe stock MySQL does not automatically strip it down like my MariaDB.

And the MySQL manual states exactly like this (for all 5.X versions): http://dev.mysql.com/doc/refman/5.7/en/myisam-storage-engine.html

The maximum key length is 1000 bytes. This can also be changed by changing the source and recompiling. For the case of a key longer than 250 bytes, a larger key block size than the default of 1024 bytes is used.

Therefore this limitation is in fact documented. Maybe @leofeyer has a non stock MySQL compiled version installed and therefore is unable to reproduce.
IMO the must be resolved though or we must state in the system requirements "Custom compiled MySQL server needed".

[BugBuster1701]

We need to solve this, everything else is nonsense.
Tell you a customer, that he has to change the hoster, because the MySQL server is "wrong" compiled?

[discordier]

To make things even worse. The "wrong" stands for "unmodified as shipped by Oracle".

[leofeyer]

Of course, I do not use custom compiled MySQL servers! Neither on my local computer (MariaDB) nor on any of our servers (some are MySQL, others are MariaDB).

So, what's the best way to solve this now? Should we add a limit to the key or drop the whole varchar(1022) thing entirely? @backbone87 Any thoughts?

[backbone87]

should leave the field as is, and define a shorter key length.
the problem is with key definiton not the field. i dont know if the install-tool can handle custom length keys yet.

[leofeyer]

Is a shorter index really a good solution? The selector is a searchable field, so what happens if the search result is not in the index?

[backbone87]

you mistake the key length. if you want full text search ops (with xtras example this applies to strings longer then 333 chars) you need full text indexes. the b-tree index used here cant optimize for LIKE %ABC% just for LIKE ABC% (left match). the only advantage the current index offers, is that hopefully no tablescan is needed because the index content can be used for text searching (but this is limited to 333 chars, so if the field is longer a tablescan is required anyway without FT index).

[backbone87]

as a sidenote: in fact (with exceptions) it is better to not use the full field length as key length on B-tree indexes if you do not do non-left match tests, because it makes the index updates more expensive. e.g. if you have a varchar(255) field for names where you do only left matches it is better to define the index with a key length <log(n) (or something like that, the optimal keylength can be calculated for different data divergency probabilities)

[leofeyer]

Maybe we should just go back to varchar(255), because we need the field to be searchable and we neither want a text field with a full text index nor a partially indexed field on which searching might require a full table scan, right?

[aschempp]

In my opinion the possibility to have much larger values than 255 chars is a higher priority than a fast search index for the backend...

[discordier]

Eh? it is searchable without fulltext index and even without any index at all...
It is only slower (in fact it is always slower unless searching for left side matches as stated by @backbone87).

Just get rid of the index.

[leofeyer]

@aschempp I'm not sure. If you look at the initial pull request, the problem could have been solved by assigning CSS classes as well. Then the selector would have been much shorter :)

[leofeyer]

in fact it is always slower unless searching for left side matches

@discordier As long as we are talking about b-tree indexes. But I agree with you.

[tristanlins]

Maybe we should just go back to varchar(255)...

No, we need to add a fulltext index instead of a regular index as @backbone87 suggested, because the original problem still exists vor a varchar(255): The regular B-tree index is not designed for in-text searches like %foo%.
Please reread @backbone87 's comments carefully!

See http://dev.mysql.com/doc/refman/5.1/de/fulltext-search.html for fulltext search in MySQL.

[leofeyer]

@tristanlins A fulltext index? Seriously? I prefer @discordier's solution.

[tristanlins]

@leofeyer what is the problem with adding a fulltext index?

[backbone87]

A b-tree index over the full field can speed up the search performance even for LIKE %ABC%, but the whole index needs to be searched (some less IO because not the whole table needs to be loaded in RAM to search the fields), but this is not that a hugh performance gain if the index file is large compared to table size.

[discordier]

In this case the table is not that large IMO... even if you push 20k entries to tl_style the table scan will still win.
It might be a different problem with conten element text etc. for which this is not the topic here.

[leofeyer]

So, we are removing the index then?

[leofeyer]

Removed in 2ae5acb.