dbDelta Not Specifying Key Length Duplicate Indexes

[charlestonsw]

Reference ticket #10404.

This is to decompose the original ticket into components. May be fixed in 4.4. Needs testing.

---

Does not match index definition. Creates duplicate indexes each time dbDelta is run:

KEY movie_type_idx (movie_type),

Does NOT create duplicate indexes:

KEY movie_type_idx (movie_type(255)),

---

Reported by: @sebet

[mnelson4]

hey @charlestonsw I'm pretty sure you meant the opposite of what you said.
KEY movie_type_idx (movie_type(255)), DOES create duplicate indexes, whereas removing the (255) does NOT. Right?

[charlestonsw]

Per @sebet on ticket #10404 entry 9... quote from his post:

---

Example:

The comparison for this index will not match and will cause a duplicate index because the length was not specified

KEY movie_type_idx (movie_type),

The comparison for this index will match and the index will not be duplicated because the length was included

KEY movie_type_idx (movie_type(255)),

---

I've not yet tested nor verified these items. Per guidance at WCUS this past weekend I simply split the #10404 "beast of a dbDelta ticket" into separate bite-sized chunks that can be tackled one-by-one.

Someone needs to double-check that I've captured the main reports attached to #10404 in the list of new tickets and then close #10404.

Many of the new "child tickets" were fixed from what I could tell but I've not yet finished the phpUnit scripting to verify.

[charlestonsw]

That said, we should check for BOTH cases and do what we can to either eliminate duplicate indexes with logic or make sure the documentation, both the old Codex page and the new code reference, are far more explicit.

Duplicate indexes is a common problem with plugins that create their own tables. It is a notable performance drain as the index count increases. Especially on older PHP & MySQL versions.

[mnelson4]

Regarding @sebet 's comment, I have an update: from my testing, dbDelta tries to create a duplicate index whenever the index length is changed. Eg if you start with
KEY movie_type_idx (movie_type(255)) (which, so long as there is no space between movie_type and (255) is actually ok) and then later switch it to KEY movie_type_idx (movie_type(10)) it will try to create a duplicate index (although it won't succeed because it will say Duplicate key name 'movie_type').
However, if you go from specifying a length (eg KEY movie_type_idx (movie_type(255))) to leaving the length UNspecified (eg KEY movie_type_idx (movie_type)) there is no problem.

ps: we can't really create unit tests that show this problem because table manipulation mysql queries will cause an implicit commit and break the unit tests... but having unit tests that show the problem (and show that it gets fixed, and doesn't get broken later on) would sure be nice.

[jdgrimes]

Replying to mnelson4:

Regarding @sebet 's comment, I have an update: from my testing, dbDelta tries to create a duplicate index whenever the index length is changed. Eg if you start with
KEY movie_type_idx (movie_type(255)) (which, so long as there is no space between movie_type and (255) is actually ok) and then later switch it to KEY movie_type_idx (movie_type(10)) it will try to create a duplicate index (although it won't succeed because it will say Duplicate key name 'movie_type').
However, if you go from specifying a length (eg KEY movie_type_idx (movie_type(255))) to leaving the length UNspecified (eg KEY movie_type_idx (movie_type)) there is no problem.

ps: we can't really create unit tests that show this problem because table manipulation mysql queries will cause an implicit commit and break the unit tests... but having unit tests that show the problem (and show that it gets fixed, and doesn't get broken later on) would sure be nice.

Actually we can create unit tests for this, and tests for dbDelta() already exist.

I suppose that the expected behavior here is that dbDelta() will automatically update the index length when it changes? And then I guess it would also remove the index length when it is no longer specified.

[mnelson4]

oh @jdgrimes I didn't realize there were tests for dbdelta already. That's helpful. I suppose the implicit commit from table changes doesn't mess them up- I thought it would.

I suppose that the expected behavior here is that dbDelta() will automatically update the index length when it changes? And then I guess it would also remove the index length when it is no longer specified.

+1

[mikejolley]

Potential fix for 34870

[mikejolley]

So I've done some work on this (https://core.trac.wordpress.org/attachment/ticket/34870/fix.34870.diff) and have made it so if it detects an existing index with the same name, the previous index is dropped first.

This is all fine and dandy, but I have one concern. Table locking.

Dropping and adding indexes can be really slow on large tables and older versions of MySQL, but I'm not sure we can do anything about it.

[mnelson4]

ya it would be nice if we could detect if the index is different or the same; if it's different we have no option but to drop it right? (regardless of how slow it might be). But if it's the same ideally we wouldn't drop it and re-create it.

[mikejolley]

@mnelson4 thats what I did in my patch above.

[mnelson4]

Oh cool. So what you did sounds good to me

[Mte90]

Very annoying with woocommerce and unit tests, any hope for 4.6?

[pento]

dbDelta() deliberately doesn't support any DROP operations, due to the potential for irrevocable data loss. In Core, any schema changes requiring a DROP operation are manually written in the upgrade_xxx() functions in wp-admin/includes/upgrade.php, plugins with custom tables should use a similar practice.

[32108] added some flexibility in the other direction - if MySQL has an index sub part defined that the schema sent to dbDelta() doesn't know about, we don't try to re-create that index. I'd be fine with a similar for this ticket - if the schema sent to dbDelta() has an index sub part that MySQL doesn't know about, don't try to re-create the index.

[pento]

Side note: if the second parameter to dbDelta() is false, it doesn't try to make the actual changes. Very useful for unit tests - see 34870.diff​.

[lemonberry]

I'm using Woocommerce & The Event Calendar and am getting duplicate registrations, of which Woocommerce are pointing to this bug.

It's hard for me to work out if it has been resolved and already included in the latest update of WP 4.7 or earlier, or not.

Can somebody help?
Thanks

[sagalbot]

I'm still having this issue in 4.7, is there any workaround? At the moment I can't run any of my tests.

Replying to Mte90:

Very annoying with woocommerce and unit tests, any hope for 4.6?

[pento]

34870.2.diff​ is a first pass at ignoring the subpart in both the SQL and the current table definition, when searching for duplicate indexes.

Could y'all please test this patch and see if it helps with your particular situations.

A couple of notes before commit: needs a deeper review to ensure I haven't missed things, or could've written it better. It also probably needs more unit tests.

[Mte90]

That patch works or in another way I don't get anymore that problems on codeception with woocommerce.

[Mte90]

The problem persist also on WP 4.7.1 but I can apply that patch to continue to work without this annoying error.

[pento]

In 39921:

dbDelta: Ignore index subparts when checking for duplicate indices.

If index lengths change in table definitions, we don't recreate the index - instead, we throw a database error, as dbDelta() tries to create a new index with the same name.

It's better to leave the index as is, MySQL doesn't have an efficient process for resizing indices, and dropping/creating is a slow process which we don't want to trigger automatically.

Fixes #34870.

[johnjamesjacoby]

dropping/creating is a slow process which we don't want to trigger automatically.

This sounds ripe for optimization.

On installations like WordPress.com and WordPress.org, automatically re-indexing will result in database lock lasting minutes to hours (been there, done that, sorry Barry.) But for the majority of WordPress installations (if the 80/20 rule approximately applies) this process likely will not be very slow at all.

We could try to calculate the number of affected rows inside of dbDelta() and set a reasonable default threshold where these kinds of alter's are allowed to run automatically.

Something like:

define( 'DB_MAX_ALTERED_ROWS', 25000 );

If a database has more rows than the maximum number that can be alter'ed, then throw a database error?

[mnelson4]

We could try to calculate the number of affected rows inside of dbDelta() and set a reasonable default threshold where these kinds of alter's are allowed to run automatically.

+1.

Or network admins could decide (there could be a constant which could control whether we want to automatically modify DB key lengths). They will probably know best if they want automatic key length modifications (and if they aren't going to do it automatically, they need to figure out some other system to do it manually).