Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#35958 closed defect (bug) (fixed)

Failing dbDelta tests due to MySQL error

Reported by: johnbillion's profile johnbillion Owned by: jeremyfelt's profile jeremyfelt
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: has-patch
Focuses: Cc:

Description

The Tests_dbDelta tests are failing on my local test setup (they have been for a while now, and I had been ignoring them due to assuming I'd messed something up).

The culprit for the failures is this MySQL error when creating the test table in Tests_dbDelta::setUp():

#1071 - Specified key was too long; max key length is 1000 bytes

The query looks like this:

CREATE TABLE wptests_dbdelta_test (
	id bigint(20) NOT NULL AUTO_INCREMENT,
	column_1 varchar(255) NOT NULL,
	PRIMARY KEY  (id),
	KEY key_1 (column_1),
	KEY compound_key (id,column_1),
	FULLTEXT KEY fulltext_key (column_1)
) ENGINE=MyISAM

Is this an environment problem or is there a bug in the tests which doesn't take into account this key length limit?

Environment:

  • MySQL server: 5.5.44
  • MySQL client: 50624 (5.6.24)
  • PHP: 5.6.99-hhvm
  • Web server: Nginx 1.9.5
  • OS: OS X 10.10.3

Attachments (4)

dbdelta-35935.patch (6.9 KB) - added by clarinetlord 8 years ago.
Patch for ticket #35958
35958-varchar-191-string-literal.diff (7.9 KB) - added by caseypatrickdriscoll 8 years ago.
35958-varchar-191-object-property.diff (8.8 KB) - added by caseypatrickdriscoll 8 years ago.
35958.diff (10.7 KB) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (20)

#1 @jdgrimes
9 years ago

I think that this is partly an issue with the tests, since a key on a VARCHAR(255) field can only be 191 in length, when using utf8mb4. So I think that the keys ought to look like this:

	KEY key_1 (column_1(191)),
	KEY compound_key (id,column_1(171)),

However, I don't get this error when I run the tests on my local machine. I believe that the reason is likely due to a difference in MySQL configuration, probably your MySQL database has a different default character set than mine. This is why WordPress always specifies the character set and collation when creating tables. But we don't do that here in these unit tests, which is likely why we're seeing inconsistent failures.

I'm no expert though, so I'm happy to be corrected on all this. :-)

#2 @ocean90
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Confirmed, happens when you have database with a utf8mb4 collation.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

@clarinetlord
8 years ago

Patch for ticket #35958

#4 @clarinetlord
8 years ago

Here's a suggested patch for this ticket. I couldn't see a good reason in the tests for the column length to be any particular byte length, so I just chose 100 because it's a common length.

#5 @clarinetlord
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#6 @caseypatrickdriscoll
8 years ago

I can confirm this on a new VVV instance with MariaDB 5.5. A similar thread is here https://core.trac.wordpress.org/ticket/35249

Since WP 4.6, all tables are set with utf8mb4 charset and collation if it is available.

https://core.trac.wordpress.org/browser/tags/4.6/src/wp-includes/wp-db.php#L758

The magic number should be 191 as that is the amount of four-byte characters available under the 767 ENGINE=MyISAM limit (191 * 4 = 764 bytes)

As string literal:
https://dl.dropboxusercontent.com/s/x30kpsxv5nzyoko/2017-02-24%20at%2018.10.png

As object property:
https://dl.dropboxusercontent.com/s/2apntbcurpwwn74/2017-02-24%20at%2018.16.png

Attached are two patches, one with the 191 string literal in every location, the other with it set as an Object Property. :)

#7 @afercia
8 years ago

I've rebuilt my VVV yesterday and getting these errors now :) If I'm not wrong, VVV changed to MariaDB on January, using utf8mb4 by default. If so, I'm guessing this is going to happen on every new install? /cc @jeremyfelt

#8 @jeremyfelt
8 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Future Release to 4.7.4
  • Version set to 4.2

This will likely be even more visible when VVV 2.0 ships, as MariaDB 10.1 is default.

We changed the default core varchar keys to varchar(191) in [31349] so that utf8mb4 could be supported. I think it makes sense to change any keys created in the dbDelta tests as well. It's probably not core's responsibility to check whether a DB can support any more than that.

35958-varchar-191-object-property.diff seems to make the most sense at first glance, though I wouldn't be opposed to 35958-varchar-191-string-literal.diff either.

#9 @clarinetlord
8 years ago

I vote for the object property patch. I think it's clearer to put up top what the purpose of that magic number is, so it's not buried in a SQL query in two separate places.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

#13 @swissspidy
8 years ago

  • Milestone changed from 4.7.4 to 4.8

#14 @desrosj
8 years ago

I too prefer the object property patch. I tested that patch locally using VVV 2.0 running MariaDB 10.1, and the unit tests no longer throw errors for me.

Output before: https://gist.github.com/desrosj/4c4f2b3a2555c1ee6030f4d4ae153339

Output after
https://cloudup.com/chqMrn6t5Xu+

@jeremyfelt
8 years ago

#15 @jeremyfelt
8 years ago

35958.diff applies the maximum index length to the key definitions rather than the field definitions, which matches the approach taken in [31349] when core was adjusted to better support utf8mb4. VARCHAR will support larger fields, the indexes have the max size.

Many of these are not entirely necessary, as the tests are using dbDelta() to determine that nothing changed and don't actually end up issuing MySQL commands. But, for consistency, I updated all of the key definitions that would require the max length to be set.

#16 @jeremyfelt
8 years ago

  • Owner set to jeremyfelt
  • Resolution set to fixed
  • Status changed from new to closed

In 40339:

Tests: Use utf8mb4 max index length when creating keys.

In [31349], core varchar column key lengths were changed from 255 to 191 to support the 767 byte index size limit on standard utf8mb4 MySQL installs. This changes the DB schema tests to match.

Props caseypatrickdriscoll, clarinetlord.
Fixes #35958.

Note: See TracTickets for help on using tickets.