Make WordPress Core

Opened 6 years ago

Last modified 18 months ago

#18609 assigned defect (bug)

term_id collisions possible with InnoDB tables and global_terms_enabled

Reported by: andrew_l Owned by: purcebr
Milestone: Future Release Priority: lowest
Severity: major Version: 3.2.1
Component: Taxonomy Keywords: has-patch needs-testing
Focuses: Cc:


This bug relates to the "global terms" feature that uses the wp_sitecategories table to store all terms used across the multi-site install. This feature is enabled by setting global_terms_enabled to "1" in the wp_sitemeta table.

I have found that when global_terms_enabled is on, and you are using InnoDB tables, it is possible for a new term insertion in the local terms table (wp_x_terms) to fail due to an ID collision. This happens because InnoDB does not handle auto_increment in the same way as MyISAM, and the code for the global terms feature does not account for this difference.

In MyISAM, any change to a value in an auto-incremented column causes the table's AUTO_INCREMENT value (next value to be used, as seen via "show table status") to change. For example, if I have a MyISAM table, and I use an update statement to change the value of an auto incremented column to a value larger than the current AUTO_INCREMENT, the AUTO_INCREMENT is automatically increased as needed, so the next insertion cannot collide.

However, InnoDB does not do this. It only changes AUTO_INCREMENT on an insert. It does not change it when a column value is increased via an update statement. You can see this in the MySQL docs, or you can see it by creating a test case, like this:

  • Create an InnoDB table with 2 columns: ID (auto_increment, primary key) and data.
  • Insert a row using an insert statement, without specifying an ID.
  • Observe that ID = 1 for that row, as expected.
  • Use an update statement to set ID = 2 on that row.
  • Insert another row without specifying an ID.
  • Observe that the insertion fails. MySQL has attempted to insert a row with ID = 2, but there is already such a row.

If you do the same steps with a MyISAM table, the insertion does not fail, because AUTO_INCREMENT was automatically changed from 2 to 3 when your update statement was executed.

Going back into the context of the global terms feature: This problem happens when a new term is created in a site. The new term is initially inserted into the local terms table for the current site (wp_x_terms). After this, the function global_terms (in ms-functions.php) is called. This function finds or creates a matching entry in the wp_sitecategories global terms table, and uses an update statement to change the term_id of the row we just inserted in wp_x_terms. When wp_x_terms is an InnoDB table, the AUTO_INCREMENT value for the table does not change.

Suppose the AUTO_INCREMENT value was 58 after the original insertion, and further suppose that 58 was (coincidentally) the ID found in wp_sitecategories and written into our new row by the update statement described above. Now suppose we are ready to insert another new term into wp_x_terms. AUTO_INCREMENT is still 58, so we attempt to insert a term with term_id = 58, but it fails because there is already a row in the table with that ID.

Fix for this would be to reorder the operations so that no row is inserted into wp_x_terms until after wp_sitecategories has been queried and updated. That way, we know the ID we need to use when inserting into wp_x_terms before that insertion takes place, so we can specify it in the insert statement rather than depending on AUTO_INCREMENT.

I have marked this as severity major, because these unpredictable term_id collisions can cause weird errors, and the only workaround I have at this time is to disable global terms and restart MySQL (to regenerate AUTO_INCREMENT values on all tables).

Attachments (1)

global_terms_auto_update_replacement.diff (954 bytes) - added by purcebr 2 years ago.

Download all attachments as: .zip

Change History (11)

#1 @unsalkorkmaz
6 years ago

  • Cc unsalkorkmaz added

#2 @wonderboymusic
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.7

Oh man - this is pretty dark, just confirmed it. Either this never happens and the ticket can be closed or IDs get changed and can blow up the auto_increment in InnoDB

#3 @pento
4 years ago

That's a pretty impressive find. It's documented InnoDB behaviour, so I wouldn't expect it to be fixed in MySQL ever.

Given how old and buggy the global terms code is (I spotted a couple of bugs in global_terms() just reading it), I'd be inclined to delete all of this code, and pretend it never happened.

#4 @wonderboymusic
4 years ago

  • Milestone changed from 3.7 to 3.8

No patch

#5 @nacin
3 years ago

  • Milestone changed from 3.8 to Future Release
  • Priority changed from normal to lowest

Would be really cool to fix, I guess. If WP.com still uses global terms, they may be the only ones.

#6 @boonebgorges
3 years ago

  • Keywords good-first-bug added

#7 @purcebr
2 years ago

What if we deleted the term from the db and recreated it with the global_id instead of updating the term with a new ID? Inserting a term properly updates the auto_incrementer and prevents this issue from cropping up in my dev environment. Not sure if this idea has been proposed already, but I've attached a patch with what I was thinking could work.

I'm not sure how we'd reorder this process without a larger refactor of global_terms(). The Local term gets created first, and the precondition for the global_terms($term_id) function is that the term_id is a created term, and the local term_id update requires an already-created global term.

#8 @ipm-frommen
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#9 @DrewAPicture
2 years ago

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

#10 @johnbillion
18 months ago

  • Keywords good-first-bug removed
Note: See TracTickets for help on using tickets.