WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 7 months ago

Last modified 5 months ago

#34870 closed defect (bug) (fixed)

dbDelta Not Specifying Key Length Duplicate Indexes

Reported by: charlestonsw Owned by: pento
Milestone: 4.8 Priority: normal
Severity: normal Version: 2.8.1
Component: Database Keywords: has-patch
Focuses: performance Cc:

Description

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

Attachments (3)

fix.34870.diff (2.1 KB) - added by mikejolley 17 months ago.
Potential fix for 34870
34870.diff (854 bytes) - added by pento 11 months ago.
34870.2.diff (4.1 KB) - added by pento 8 months ago.

Download all attachments as: .zip

Change History (26)

#1 @mnelson4
21 months ago

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?

#2 @charlestonsw
21 months ago

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.

#3 @charlestonsw
21 months ago

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.

#4 follow-up: @mnelson4
21 months ago

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.

#5 in reply to: ↑ 4 @jdgrimes
20 months ago

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.

#6 @mnelson4
20 months ago

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
17 months ago

Potential fix for 34870

#7 @mikejolley
17 months ago

  • Keywords has-patch needs-testing needs-unit-tests dev-feedback added

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.

#8 @mnelson4
17 months ago

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.

#9 @mikejolley
17 months ago

@mnelson4 thats what I did in my patch above.

#10 @mnelson4
17 months ago

Oh cool. So what you did sounds good to me

#11 follow-up: @Mte90
16 months ago

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

#12 @pento
11 months ago

  • Keywords needs-refresh added; needs-testing dev-feedback removed

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
11 months ago

#13 @pento
11 months ago

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.

#14 @lemonberry
9 months ago

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

#15 in reply to: ↑ 11 @sagalbot
8 months ago

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
8 months ago

#16 @pento
8 months ago

  • Keywords needs-testing added; needs-refresh removed
  • Milestone changed from Awaiting Review to 4.8

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.

#17 @Mte90
8 months ago

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

#18 @Mte90
7 months ago

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

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


7 months ago

#20 @pento
7 months ago

  • Keywords needs-unit-tests needs-testing removed
  • Owner set to pento
  • Status changed from new to assigned

#21 @pento
7 months ago

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

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.

#22 @johnjamesjacoby
5 months ago

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?

Last edited 5 months ago by johnjamesjacoby (previous) (diff)

#23 @mnelson4
5 months ago

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).

Note: See TracTickets for help on using tickets.