term_id collisions possible with InnoDB tables and global_terms_enabled

[andrew_l]

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

[wonderboymusic]

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

[pento]

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.

[wonderboymusic]

No patch

[nacin]

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

[purcebr]

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.

[desrosj]

This one needs some testing on the current version of WordPress (currently 6.0.1) and newer versions of MySQL to see if anything has changed.

There have been very few global terms related tickets since this one (#46297, #55979 to name a few).

@pento does .com still use global terms?

[peterwilsoncc]

@desrosj Pento is currently on sabbatical so unlikely to respond any time soon. I'll ask another automattican to check.

[dd32]

Replying to desrosj:

does .com still use global terms?

I believe it still uses a form of Global Terms, although I don't believe it's using the WordPress Global Terms code directly - it appears a custom plugin is in use (and likely was before the code was in core I think).

I know it was disabled on a subset of sites around the time of term splitting, but I don't believe WordPress.com ever removed it entirely.

Replying to nacin:

If WP.com still uses global terms, they may be the only ones.

9 years later, I bet that holds true; I'd be surprised if many sites remain that use global terms intentionally.

[peterwilsoncc]

The only time I've heard of someone outside of this ticket using global terms was to set up truly global terms: meaning each site used the network's terms table and displayed the same list of terms on all sites.

I think the KISS approach of the original patch is the best fix for this. I've refreshed it in 18609.diff​ and continued to use an UPDATE if the new ID is unlikely to modify the auto increment value.

... I wouldn't expect it to be fixed in MySQL ever.

Turns out it was fixed in 8.0 :)

Viably, I think if code similar to the patch isn't committed any time soon this ticket ought to be closed wontfix.

[desrosj]

I'm going to mark this as a wontfix. I've started a discussion over on #21734 (see my latest comment ticket:21734#comment:19) to explore deprecating the remaining related code still in Core today.

Global terms was "soft" deprecated in WP 3.0 shortly before this ticket was created, and is very broken in modern WordPress (shared terms and term meta do not work at all). Anyone still using this "feature" should consider finding an alternative way to accomplish their goals.