Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#41716 closed defect (bug) (fixed)

Check for utf8mb4 incorrectly assumes auto-truncating indexes

Reported by: straussd's profile straussd Owned by: pento's profile pento
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.2
Component: Database Keywords: has-patch
Focuses: Cc:

Description (last modified by danielbachhuber)

The current Tests_dbDelta::test_truncated_index test (from #31869, r32108) makes an incorrect assumption about MySQL/MariaDB configuration. Specifically, it assumes that presence of utf8mb4 capabilities means that either Barracuda (the necessary InnoDB file format to pass the test) is the default file format for all newly created tables (or that the database silently truncates indexes).

This is not always the case for MariaDB 10.0 or 10.1 and is probably an issue for any MySQL<5.7.7 or MariaDB<10.2 with the following configuration:

innodb_file_per_table=true
innodb_file_format=Barracuda
innodb_large_prefix=true

Enabling innodb_large_prefix (which depends on the other two options) causes the database to stop silently truncating long indexes, but the database also won't simply switch to Barracuda for long indexes to make creation succeed, either.

Despite "innodb_file_format" being documented in MariaDB as "File format for new InnoDB tables," it only takes effect for tables explicitly created with a Barracuda-based row format. (The MySQL documentation is better because it doesn't suggest that it applies to all new tables.)

I see two possible fixes:

(1) Expand the criteria for markTestSkipped to include creating a table and verifying that it uses a Barracuda row format (i.e. COMPRESSED}} or {{{DYNAMIC). This will indicate that the database uses Barracuda for all new tables.

(2) Explicitly specify the row format for the table creation test itself. This would require adding ROW_FORMAT=DYNAMIC to the existing CREATE TABLE. This would have no effect on the latest MySQL and MariaDB releases, which default to that anyway.

In a related issue, the test is misnamed based on the behavior it tests. In older MySQL releases (and with no long index support enabled), the test succeeds if the database silently truncates the long index to just a prefix. However, in the latest releases, no truncation occurs at all. The reason the test passes in MySQL 5.7 and MariaDB 10.2 is a new default of long index support and Barracuda. So, at a minimum, the test should be renamed.

Attachments (4)

41716.1.diff (597 bytes) - added by danielbachhuber 7 years ago.
Add ROW_FORMAT=DYNAMIC to CREATE TABLE query
41716.diff (1.0 KB) - added by pento 7 years ago.
41716.2.diff (1.3 KB) - added by danielbachhuber 7 years ago.
41716.3.diff (1.2 KB) - added by pento 7 years ago.

Download all attachments as: .zip

Change History (14)

This ticket was mentioned in Slack in #hosting-community by danielbachhuber. View the logs.


7 years ago

#2 @danielbachhuber
7 years ago

  • Description modified (diff)

#3 @danielbachhuber
7 years ago

@pento @jdgrimes Do either of you have opinions on the best way to approach this?

@danielbachhuber
7 years ago

Add ROW_FORMAT=DYNAMIC to CREATE TABLE query

#4 @danielbachhuber
7 years ago

Added a patch for the second option, appending ROW_FORMAT=DYNAMIC to the CREATE TABLE query.

If options 1 and 2 are equal, then choosing the second option avoids an unnecessary table schema lookup query.

@pento
7 years ago

#5 @pento
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.9
  • Owner set to pento
  • Status changed from new to assigned
  • Version changed from trunk to 4.2

Well, this was interesting. I was getting all kinds of weirdness due to it the unit tests transforming it into a temporary table, so I've explicitly disable that for this test.

Because the point of this test is to ensure that dbDelta() ignores if an index has been silently truncated, 41716.diff forces the ROW_FORMAT to COMPACT, which should truncate the index under every version of MySQL and MariaDB, regardless of server config.

@straussd, @danielbachhuber: Does this fix the problems you're running into?

#6 @danielbachhuber
7 years ago

@pento When running 41716.diff on Pantheon, I see this error in the test log:

<div id="error"><p class="wpdberror"><strong>WordPress database error:</strong> [Index column size too large. The maximum column size is 767 bytes.]<br /><code>
			CREATE TABLE wptests__test_truncated_index (
				a varchar(255) COLLATE utf8mb4_unicode_ci,
				KEY a_key (a)
			) ENGINE=InnoDB ROW_FORMAT=COMPACT</code></p></div>

And this test failure:

1) Tests_dbDelta::test_truncated_index
Failed asserting that Array &0 (
    'wptests__test_truncated_index' => 'Created table wptests__test_truncated_index'
) is identical to Array &0 ().

I see the same test failure when running the test locally.

$ mysql --version
mysql  Ver 15.1 Distrib 10.2.8-MariaDB, for osx10.12 (x86_64) using readline 5.1

My understanding is that the table is not being created when the index column size is too large. Subsequently, dbDelta reports the table needs to be created when the test expects the table is already created.

@pento
7 years ago

#7 @pento
7 years ago

Okay, now we might be getting somewhere!

Testing against MariaDB 10.1, with the MySQL config described in the original ticket:

innodb_file_per_table=true
innodb_file_format=Barracuda
innodb_large_prefix=true

I was able to reproduce the error when creating the table. So, in 41716.2.diff, I'm setting ROW_FORMAT=DYNAMIC on the CREATE TABLE, which causes the following behaviour:

  • On servers with innodb_file_format=Antelope, the ROW_FORMAT is ignored, and defaults to COMPACT.
  • On MariaDB 10.1 (and presumably MySQL 5.6, we'll find out when Travis runs it), with the above config, the test is skipped, because it uses DYNAMIC, so doesn't truncate the index.
  • On MariaDB 10.2, the test is skipped.

Could you try this patch out, and see where we're at?

#8 @straussd
7 years ago

I don't have my test setup at my fingertips, but the explanation and patch look correct. I will have an opportunity to test again when I'm back from travel. That should get us results later this week -- if @danielbachhuber doesn't beat me to it.

#9 @danielbachhuber
7 years ago

@pento Test is skipped as expected locally and on Pantheon. :shipit:

#10 @pento
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41818:

Database: Fix a test failing on MySQL 5.7 and MariaDB 10.2.

On newer versions of MySQL, an error was being thrown when creating a table with an index that we wanted to be silently truncated.

To avoid this, the test now tries to use a newer InnoDB file format where available, and skips the test when that happens.

Props pento, danielbachhuber, straussd.
Fixes #41716.

Note: See TracTickets for help on using tickets.