WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 11 months ago

#31869 closed defect (bug) (fixed)

dbDelta doesn't allow for automatically truncated indexes

Reported by: pento Owned by: pento
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

When a table is created with too large an index, MySQL will automatically truncate it. For example:

CREATE TABLE foo (
  a VARCHAR(255) CHARACTER SET utf8mb4,
  INDEX (a)
);

MySQL will automatically truncate the index on a to be 191 characters.

When dbDelta() checks SHOW INDEX FROM foo, it'll see that the index is 191 characters, when this wasn't specified in the CREATE TABLE statement, so it'll try and recreate the index that already exists.

This bug has probably existed since forever, but will become more prominent now that utf8mb4 is a thing.

Attachments (3)

31869.diff (2.0 KB) - added by pento 2 years ago.
31869.2.diff (1.5 KB) - added by pento 2 years ago.
31869.3.diff (1.5 KB) - added by pento 2 years ago.

Download all attachments as: .zip

Change History (22)

#1 @pento
2 years ago

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

#2 follow-up: @pento
2 years ago

A couple of ideas on how to fix this:

  1. When the character set is utf8mb4, 191 becomes a magic number, and is treated the same as an empty subpart. Magic numbers are kind of eww.
  1. Remove the subparts from the both the original CREATE TABLE and the generated index definition - we're really only interested in the index existing, not the size of it. This may change in the future, though.

#3 in reply to: ↑ 2 @jdgrimes
2 years ago

Replying to pento:

  1. Remove the subparts from the both the original CREATE TABLE and the generated index definition - we're really only interested in the index existing, not the size of it. This may change in the future, though.

Yeah, I think what needs to happen is the check for whether an index exists needs to be split from the check of whether it is the same length. If the lengths are different, the index may need to be updated (unless the current length is the default for the character set being used, and no length is specified). I'm not an expert on how MySQL indexes work, however. :-)

#4 @nacin
2 years ago

I imagine we have increased an index before. Or rather, I imagine someone has used dbDelta to increase an index before. Not sure we'd want to break that (though breaking it would not be a big deal).

#5 @pento
2 years ago

There's an existing work around - to define the index sizes in CREATE TABLE as 191 characters, so that MySQL doesn't automatically change their size, that's what we've done in core.

I don't know how people would've used dbDelta to change indexes in the past - dbDelta never drops indexes, it only adds them.

@pento
2 years ago

#6 follow-up: @pento
2 years ago

  • Keywords has-patch added; needs-patch removed

31869.diff adds a second check for columns that have an index length in the DB, but not in the CREATE TABLE statement. It throws a _doing_it_wrong() warning for any that it finds, but doesn't try to add the existing index.

@DrewAPicture - ping for a new string added in this patch. This patch (or something like it) needs to happen in 4.2, but I'll leave it to your discretion as to how/if we handle a new string after string freeze.

#7 in reply to: ↑ 6 @DrewAPicture
2 years ago

  • Keywords 4.2-strings added

Replying to pento:

@DrewAPicture - ping for a new string added in this patch. This patch (or something like it) needs to happen in 4.2, but I'll leave it to your discretion as to how/if we handle a new string after string freeze.

Let's hold on to the new string until a little bit closer to RC1. I'm sure we'll have other string changes, and I'd kind of like to bundle them together.

@pento
2 years ago

#8 @pento
2 years ago

31869.2.diff cleans up the patch a bit.

I'm not wild about the error message, though.

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


2 years ago

#11 @pento
2 years ago

Nope. #31388 was caused by an index not being there when it should be. This is caused by an index having a different sized prefix than defined in the CREATE TABLE statement.

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


2 years ago

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


2 years ago

@pento
2 years ago

#14 @pento
2 years ago

I'm happy with the state of the the string in 31869.3.diff - who knew that a one word change could make such a difference?

My concern with the functionality of the patch is that it really only handles one specific case - when the CREATE TABLE statement doesn't specify a subpart, but MySQL shows that there is a subpart (most likely because it had to trim an index that was too long).

Cases is doesn't handle:

  • When the CREATE TABLE has a subpart specified (say, you manually specified a subpart of 255, but MySQL trims it down because of utf8mb4) that differs from the subpart that MySQL shows.
  • When the CREATE TABLE has a subpart specified, but MySQL does not (I'm not sure how this would happen).

I don't know if we need to handle those extra cases, though.

#15 @pento
2 years ago

  • Keywords 4.2-strings removed

I've discussed this with @dd32, and given it some more thought. I think that our best route is remove the _doing_it_wrong() message, and just treat an undefined index subpart as being okay, if it turns out MySQL does have an index subpart.

I've changed the fix so that we can easily expand the set of "equivalent" index definitions, if more crop up.

#16 @pento
2 years ago

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

In 32108:

When dbDelta() is checking whether an index is defined in a CREATE TABLE statement, don't worry if MySQL has a subpart defined on an index, but the CREATE TABLE doesn't.

Fixes #31869.

#17 @jdgrimes
2 years ago

Thanks @pento. I think [32108] the right way to go.

#18 @pento
11 months ago

#30795 was marked as a duplicate.

#19 @pento
11 months ago

#32314 was marked as a duplicate.

Note: See TracTickets for help on using tickets.