WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#36748 closed defect (bug) (fixed)

Updating tables to utf8mb4 causes some columns to change type unexpectedly

Reported by: tollmanz Owned by: pento
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: fixed-major
Focuses: performance Cc:
PR Number:

Description

When updating WordPress to a version greater than 4.2 from a version less than 4.2 and invoking the utf8mb4 DB upgrade, it causes some text column types to unexpectedly change in order to accommodate the space for the extra bytes. This change is unintended and causes the DB to get out of sync with the specified schema since the dbDelta update runs prior to the utf8mb4 updates.

Once this has happened, WordPress will fix the issue once the next update occurs (e.g., if the issue is introduced updating 4.1.10 to 4.4.2, the correct schema will be reapplied when updating to 4.5.1). This is a major annoyance as, in my experience, this schema correction is hanging the DB for ~6 mins to perform this update.

Mathia Bynens documented this issue on an excellent post on upgrading MySQL for utf8mb4 support (https://mathiasbynens.be/notes/mysql-utf8mb4):

Also of note when converting a table to utf8mb4 from utf8: if you have a column that is TEXT, MySQL
 will automagically promote that column to MEDIUMTEXT to accomodate for the additional storage
 space needed for the new encoding. I only tested this with TEXT and assume it is similar with
TINYTEXT etc.

For WordPress, I've found the following issues:

| column                     | should be | changed to |  
| -------------------------- | --------- | ---------- |
| wp_posts.post_title        | text      | mediumtext |
| wp_posts.post_excerpt      | text      | mediumtext |
| wp_posts.to_ping           | text      | mediumtext |
| wp_posts.pinged            | text      | mediumtext |
| wp_comments.comment_author | tinytext  | text       |
| wp_links.link_notes        | text      | mediumtext |

To reproduce:

  1. Install WP version 4.1.10
  2. Upgrade to WP version 4.4.2, making sure to run the DB upgrade routine
  3. Notice the columns mentioned above do not match the schema
  4. Upgrade to WP version 4.5.1, making sure to run the DB upgrade routine
  5. Notice that the columns are correct according to the schema

Note that WordPress will not correct the schema during a minor release (e.g., 4.4.1 to 4.4.2), only during a major release.

Please see these Travis CI builds that show the bug in action:

Change History (16)

#1 @pento
4 years ago

  • Focuses performance added
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.5.2

I'm inclined to fix this by preventing dbDelta() from downgrading the size of a *text (or *blob) field.

We're already allowing for custom sized columns in the WPDB preflight checks, so this is mostly about preventing an unnecessary ALTER TABLE.

Because of the significant performance implications, this will need to be backported to the 4.2 branch, in preparation for major release auto updates.

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


3 years ago

#3 @adamsilverstein
3 years ago

  • Owner set to pento
  • Status changed from new to assigned

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


3 years ago

#5 @pento
3 years ago

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

In 37525:

Database: dbDelta() will no longer try to downgrade the size of TEXT and BLOB columns.

When upgrading to utf8mb4, TEXT fields will be upgraded to MEDIUMTEXT (and likewise for all other *TEXT and *BLOB fields). This is to allow for the additional space requirements of utf8mb4.

On the subsequent upgrade, after the utf8mb4 upgrade, dbDelta() would try and downgrade the fields to their original size again. At best, this it a waste of time, at worst, this could truncate any data larger than the original size. There's no harm in leaving them at their original size, so let's do that.

Fixes #36748.

#6 @pento
3 years ago

  • Keywords fixed-ma added; needs-patch needs-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[37525] will need to be backported to the 4.2-4.5 branches.

#7 @ocean90
3 years ago

  • Keywords fixed-major added; fixed-ma removed

#8 @tollmanz
3 years ago

@pento Should the DB schema also be updated? Seems like it would be ideal for the schema to match what the database will actually look like.

#9 @pento
3 years ago

I'm inclined to leave it as is, particularly as many sites have already gone through the upgrade-then-downsize dance. Upsizing them again is just unnecessary downtime.

#10 @ocean90
3 years ago

In 37606:

Database: dbDelta() will no longer try to downgrade the size of TEXT and BLOB columns.

When upgrading to utf8mb4, TEXT fields will be upgraded to MEDIUMTEXT (and likewise for all other *TEXT and *BLOB fields). This is to allow for the additional space requirements of utf8mb4.

On the subsequent upgrade, after the utf8mb4 upgrade, dbDelta() would try and downgrade the fields to their original size again. At best, this it a waste of time, at worst, this could truncate any data larger than the original size. There's no harm in leaving them at their original size, so let's do that.

Merge of [37525] to the 4.5 branch.

Props pento.
See #36748.

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


3 years ago

#12 @pento
3 years ago

In 37936:

Database: dbDelta() will no longer try to downgrade the size of TEXT and BLOB columns.

When upgrading to utf8mb4, TEXT fields will be upgraded to MEDIUMTEXT (and likewise for all other *TEXT and *BLOB fields). This is to allow for the additional space requirements of utf8mb4.

On the subsequent upgrade, dbDelta() would try and downgrade the fields to their original size again. At best, this it a waste of time, at worst, this could truncate any data larger than the original size. There's no harm in leaving them at their new size, so let's do that.

This also fixes a typo in the dbDelta() tests.

Merge of [37525] to the 4.4 branch.
Partial merge of [36552] to the 4.4 branch.

See #36748.

#13 @pento
3 years ago

In 37938:

Database: dbDelta() will no longer try to downgrade the size of TEXT and BLOB columns.

When upgrading to utf8mb4, TEXT fields will be upgraded to MEDIUMTEXT (and likewise for all other *TEXT and *BLOB fields). This is to allow for the additional space requirements of utf8mb4.

On the subsequent upgrade, dbDelta() would try and downgrade the fields to their original size again. At best, this it a waste of time, at worst, this could truncate any data larger than the original size. There's no harm in leaving them at their new size, so let's do that.

The FULLTEXT indexes are removed from the tests, as dbDelta()'s FULLTEXT support was added in WordPress 4.4.

This also fixes a typo in the dbDelta() tests.

Merge of [37525] to the 4.3 branch.
Partial merge of [36552] to the 4.3 branch.

See #36748.

#14 @pento
3 years ago

In 37939:

Database: dbDelta() will no longer try to downgrade the size of TEXT and BLOB columns.

When upgrading to utf8mb4, TEXT fields will be upgraded to MEDIUMTEXT (and likewise for all other *TEXT and *BLOB fields). This is to allow for the additional space requirements of utf8mb4.

On the subsequent upgrade, dbDelta() would try and downgrade the fields to their original size again. At best, this it a waste of time, at worst, this could truncate any data larger than the original size. There's no harm in leaving them at their new size, so let's do that.

The FULLTEXT indexes are removed from the tests, as dbDelta()'s FULLTEXT support was added in WordPress 4.4.

This also includes the setUp() and tearDown() parts of [32270], to allow the tests to run, and fixes a typo them.

Merge of [37525] to the 4.2 branch.
Partial merge of [36552] to the 4.2 branch.
Partial merge of [32270] to the 4.2 branch.

See #36748.

#15 @pento
3 years ago

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

We're done here.

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


3 years ago

Note: See TracTickets for help on using tickets.