WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#30261 closed enhancement (fixed)

Split all existing shared taxonomy terms on WP upgrade

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.3 Priority: high
Severity: blocker Version:
Component: Taxonomy Keywords:
Focuses: Cc:

Description

As of [30241], updating an existing shared term (via wp_update_term()) will force that term to be split into two separate terms. This effectively solves the problem of updates to a term in one taxonomy causing updates to take place in other taxonomies as well. (See #5809.) However, the ongoing existence of shared terms in the database stands in the way of other improvements to the taxonomy component, such as the introduction of termmeta. So we should figure out a way to force *all* existing terms to be split.

I assume that this will happen during a WordPress upgrade, perhaps in the update_to_420() function. The update can use the existing _split_shared_term() function, but it should probably skip the per-term cache flushing (which could slow down the process considerably on some setups). Or it might be faster to write a new bulk migration routine.

Attachments (21)

30261.diff (6.7 KB) - added by boonebgorges 3 years ago.
30261.2.diff (7.1 KB) - added by boonebgorges 3 years ago.
30261.3.diff (8.2 KB) - added by boonebgorges 2 years ago.
30261.4.diff (534 bytes) - added by boonebgorges 2 years ago.
30261.5.diff (7.2 KB) - added by boonebgorges 2 years ago.
30261.6.diff (9.3 KB) - added by boonebgorges 2 years ago.
30261.7.diff (7.7 KB) - added by boonebgorges 2 years ago.
30261.8.diff (7.2 KB) - added by peterwilsoncc 2 years ago.
30261.9.diff (7.2 KB) - added by peterwilsoncc 2 years ago.
fixes typo in .8.diff
30261.10.diff (7.5 KB) - added by pento 2 years ago.
30261.11.diff (7.7 KB) - added by pento 2 years ago.
30261.12.diff (795 bytes) - added by Chouby 2 years ago.
30261.13.diff (8.4 KB) - added by pento 2 years ago.
30261.14.diff (8.5 KB) - added by boonebgorges 2 years ago.
30261.15.diff (8.6 KB) - added by boonebgorges 2 years ago.
30261.16.diff (9.0 KB) - added by pento 2 years ago.
30261.17.diff (822 bytes) - added by Chouby 2 years ago.
30261.18.diff (344 bytes) - added by ocean90 2 years ago.
30261.19.diff (1.1 KB) - added by boonebgorges 2 years ago.
30261.20.diff (344 bytes) - added by boonebgorges 2 years ago.
30261.21.diff (832 bytes) - added by boonebgorges 2 years ago.

Download all attachments as: .zip

Change History (112)

#1 @boonebgorges
3 years ago

This ticket blocks #10142.

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


3 years ago

#3 @marsjaninzmarsa
3 years ago

So what? 4.3 milestone?

#4 @boonebgorges
3 years ago

  • Milestone changed from Future Release to 4.3

Yes, let's try to do it for 4.3.

#5 @obenland
3 years ago

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

@boonebgorges
3 years ago

#6 @boonebgorges
3 years ago

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

30261.diff is a first attempt at an upgrade routine. The logic is pretty straightforward, but here are a few implementation notes for anyone looking at it:

  1. I've put it into a separate function _upgrade_430_split_all_shared_terms() for easier testing.
  2. I wrote a wp-cli command to facilitate testing https://gist.github.com/boonebgorges/2beb1379569c3bd85803
  3. My main concern was performance:
    • The HAVING...GROUP BY query allows us to pull up only those terms that are shared.
    • I've changed _split_shared_term() so that it will accept term/term_taxonomy objects rather than just IDs. This saves some database queries.
    • In my profiling, the process of recording the '_split_terms' option accounted for 70-80% of the total execution time. So I've added a $record toggle, and put the recording logic into the upgrade routine. The only potential problem is that plugins may expect '_split_terms' to be up-to-date when hooking to 'split_shared_term', which won't be the case during the upgrade routine. Not a dealbreaker, but something that should be documented.

With these modifications, I think it's running reasonably fast. On a local installation with ~15000 terms, of which about 1000 are shared between two or more taxonomies, the upgrade runs in 6-8 seconds. I assume that the vast majority of WP installations will have far fewer shared terms than this, and those that have much more probably already have failsafes in place to ensure that upgrade routines don't time out. It would be great to have feedback on this point from people who manage the infrastructure for very large WP installations.

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


3 years ago

@boonebgorges
3 years ago

#8 @boonebgorges
3 years ago

30261.2.diff adds more precise cache management.

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


3 years ago

@boonebgorges
2 years ago

#10 @boonebgorges
2 years ago

30261.3.diff moves the upgrade routine into upgrade_430() and does the necessary db_version bump. You can still use this gist to create a bunch of shared terms, and then test the upgrade by rolling back your 'db_version' option as necessary and then running the database upgrade routine.

#11 follow-up: @batmoo
2 years ago

Any thoughts/discussion on using the multisite-like upgrade routine instead (i.e. process terms in small batches) to avoid the potential timeout issue?

#12 in reply to: ↑ 11 @boonebgorges
2 years ago

Replying to batmoo:

Any thoughts/discussion on using the multisite-like upgrade routine instead (i.e. process terms in small batches) to avoid the potential timeout issue?

To my knowledge, WP updates have never used a system like this, so it'd have to be built for scratch. There'd be a number of complications to consider. For example, the Multisite network upgrader would have to be aware of the multi-pageload per-site updates.

In this case, it's highly likely that only a very small percentage of sites will have more than a handful of shared terms. And those that do probably have some protocol in place for running WP database updates (see, eg, the utf8mb4 update in 4.2). So it seems to me like it wouldn't be a good use of effort to build a multi-stage upgrader just for this specific issue.

#13 @dd32
2 years ago

Additionally, most people never see that "Database upgrade" page as it's done internally by the updater. Yes, we could add multi-part support to that, but that doesn't seem ideal to me.

#14 @boonebgorges
2 years ago

In 32813:

Performance enhancements for _split_shared_term().

  • Introduce a $record parameter, which defaults to true. When set to false, _split_shared_term() will not keep a record of split term data in wp_options. The judicious use of this flag can greatly improve performance when processing shared terms in batches.
  • Allow term/tt objects to be passed to the $term_id and $term_taxonomy_id parameters. This has the potential to save database queries when the objects are already available.

See #30261.

#15 @boonebgorges
2 years ago

In 32814:

Split all shared taxonomy terms on upgrade to 4.3.

Dear Shared Terms, Welcome to Splitsville. Population: You.

See #30261.

#16 follow-up: @dd32
2 years ago

The function needs to break early when no shared terms exist, I got this SQL error upon upgrade :)

SELECT * FROM wp_term_taxonomy WHERE `term_id` IN ()

#17 in reply to: ↑ 16 @boonebgorges
2 years ago

Replying to dd32:

The function needs to break early when no shared terms exist, I got this SQL error upon upgrade :)

SELECT * FROM wp_term_taxonomy WHERE `term_id` IN ()

Oops ;)

#18 @boonebgorges
2 years ago

In 32819:

Bail out of 4.3 term splitting routine when no shared terms are found.

Props dd32.
See #30261.

#19 follow-up: @obenland
2 years ago

@boonebgorges, anything else needed here?

#20 in reply to: ↑ 19 @boonebgorges
2 years ago

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

Replying to obenland:

@boonebgorges, anything else needed here?

Nothing specific. Let's close it, and we'll see what rolls in once this is in the wild.

#21 @Chouby
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'd like to reopen this ticket as terms are not splitted when upgrading from an old WP version. Let's assume that we are upgrading from WP 4.0 (db_version: 29630), _split_shared_term bails at

	if ( $current_db_version < 30133 ) {
		return $term_id;
	}

The db version is then updated and there will be no other attempt to execute split_all_shared_terms.

#22 @Chouby
2 years ago

Testing further, I found a worse issue. Unless, I missed something, the 'split_shared_term' action is inoperant since plugins are not loaded during the upgrade. I guess that a few other plugins than mine are using this action since WP 4.2 to upgrade their own data when terms are splitted.

#23 follow-up: @boonebgorges
2 years ago

Chouby, thanks for the report.

Unless, I missed something, the 'split_shared_term' action is inoperant since plugins are not loaded during the upgrade.

Plugins are loaded during upgrades. wp_upgrade() is called from wp-admin/upgrade.php, which calls the full WP bootstrap via wp-load.php. If you were seeing failures in your tests, it's probably because (a) of the db_version problem you mentioned, or (b) because your plugin doesn't register its taxonomies early enough to be caught during upgrade routines.

I'd like to reopen this ticket as terms are not splitted when upgrading from an old WP version.

This is correct - thank you very much for catching it. At some point, the db_version check here can be removed altogether, but for the time being, a safe fix is to add a WP_INSTALLING check alongside the db_version check. See 30261.4.diff.

@boonebgorges
2 years ago

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


2 years ago

#25 in reply to: ↑ 23 ; follow-up: @Chouby
2 years ago

Plugins are loaded during upgrades.

wp_get_active_and_valid_plugins() is checking for WP_INSTALLING and returns an empty array when it is true.

I guess it is generally safer not to load plugins during a WP upgrade. Except in this specific case.

#26 @boonebgorges
2 years ago

Chouby - You are correct. I'd missed this because I'd been using an mu-plugin for my testing of the upgrade script.

This complication means that term splitting (or at least, the firing of hooks) needs to be moved to the pageload following the upgrade. See 30261.5.diff for an example of how this would work. I'm not a fan of this pattern (the 'admin_init' check) but there's some precedent - see 'link_manager_enabled'.

Would really appreciate feedback on the approach in the patch. I'm particularly concerned about introducing race conditions, but I've tried to mitigate this with the 'update_core' cap check, as well as by setting the 'shared_terms_split' flag *before* running the split routine.

@boonebgorges
2 years ago

#27 in reply to: ↑ 25 @dd32
2 years ago

Replying to Chouby:

Plugins are loaded during upgrades.

wp_get_active_and_valid_plugins() is checking for WP_INSTALLING and returns an empty array when it is true.

I guess it is generally safer not to load plugins during a WP upgrade. Except in this specific case.

I have a feeling that was never supposed to apply to the DB Upgrade step.. I wonder what would break if the define was moved to after plugins are loaded..

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


2 years ago

@boonebgorges
2 years ago

#29 follow-up: @boonebgorges
2 years ago

@dd32 pointed out that the solution previously suggested could result in a lag when viewing the about.php page, which seems like an acceptable way to introduce someone to the new version of WP. The 'admin_init' trick also doesn't work reliably during multisite upgrade routines. Moreover, @pento and @dd32 both pointed out that it would be nice to have some automatic recovery or retry system, in case the PHP process times out during the splitting routine.

30261.6.diff is an attempt to address all these issues. During upgrade_430(), an HTTP request is sent to upgrade.php?step=split_shared_terms. I've modified upgrade.php so that WP_INSTALLING is not defined during the split_shared_terms request. (This is the ugliest part of the patch, but we don't have a better system for asynchronous loopback requests - ajax-actions seems like the wrong place, and with admin-ajax.php we have to deal with nopriv vs priv issues, since the loopback request doesn't have a WP auth cookie.) So, during a normal upgrade, term splitting happens in an external request, in which all plugins are run. This works as expected in both multisite and non-multisite environments.

Second, I kept an 'admin_init' hook in there. This ensures that, if the splitting process is killed for some reason (eg, due to a timeout), it will retry the next time an Administrator visits the admin. When split_all_shared_terms() figures out that it's finished splitting all shared terms - either by finishing its foreach loop, or by detecting that there's nothing to split - it will set a flag in the database that will prevent the routine from being run again in the future. This should be pretty resiliant.

Note that 30261.6.diff also contains the fix to the $this bug in #33206, which will allow you to test during a network upgrade.

Feedback and testing is sorely needed, especially from plugin authors who are affected by term splitting. Here's how I've been testing:

  • Install WP 4.2.x to a fresh database
  • Optional: to test in multisite, convert installation to multisite and create a new site or two
  • Generate some shared terms (on all sites). I've been using this wp-cli command: https://gist.github.com/boonebgorges/2beb1379569c3bd85803
  • (Network-)Activate plugins that hook to 'split_shared_term'. I wrote a dummy plugin that hooks to 'split_shared_term' and writes to an error log.
  • Replace 4.2.x files with trunk files (svn switch, git checkout, whatever) and apply 30261.6.diff
  • Run the db upgrade routine, as appropriate for your config (multisite vs non)
  • Check that terms have been split, and that your plugin has detected the splits

@dd32 @pento I would especially value your feedback on:

  • Whether the added overhead of the loopback HTTP request is going to cause problems, particularly during network upgrades
  • Whether the failure recovery technique (an 'admin_init' callback, which sets an "all done" flag when it detects that the migration is complete) seems OK to you
  • Whether you can think of a less horrid way of allowing a non-authenticated term-splitting routine than my upgrade.php hack

#30 @boonebgorges
2 years ago

  • Severity changed from normal to blocker

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


2 years ago

#32 @obenland
2 years ago

  • Priority changed from normal to high

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


2 years ago

#34 @dd32
2 years ago

The major thing that springs to mind here with the retrying, is that the back-compat option _split_terms will become invalid/only partially filled if the process dies part-way (It'll contain the last successful runs data).

If no users on a site have the update_core capability (say because it's disabled as the site is using version control), and the loopback HTTP request fails, then the split term functionality won't be run then either.

I'm really curious if we can just remove WP_INSTALLING from the upgrade routines instead and just have it work how we expected, the only thing I can see is that it'll cause get/update_option() to utilise the cache (and update the caches).
That won't help with retrying the upgrade though, and I fear it'll cause some kind of unexpected behaviour in a plugin somewhere..

I'm hoping that the number of shared terms on sites is actually fairly low, we only see a handful of reports of shared-term bugs, so it's bound to be a small percentage of sites.. which is why I'm hoping that having to deal with retrying the process could possibly be dealt with later..

#35 in reply to: ↑ 29 @netweb
2 years ago

Just did this:

  • Installed WP 4.2.4 and fresh DB
  • Created sub-directory multisite setup with two additional sub-dir sites alongside the primary site
  • Added and network activated "WP Multi-Network" and Jetpack plugin and "adirondack" theme for (Jetpack featured-content theme compat)
  • Created an additional two networks
  • Used the WP-CLI plugin to created shared terms in each of the 5 sites wp bbg generate_shared_terms --url=test.dev/site3 etc
  • Created some posts with the "featured" tag for Jetpack, Created "featured" category and "featured" tags
  • Switched to /trunk and applied 30261.6.diff​
  • Ran the upgrade, network upgrade, and subsequent network upgrades
  • All the terms in all 5 sites without error and worked as expected (Including Jetpack "featured" posts) :)
Last edited 2 years ago by netweb (previous) (diff)

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


2 years ago

#37 follow-up: @Chouby
2 years ago

I tested 30261.6.diff and there is still a problem when upgrading from an older version (ex: 4.0):

	if ( $current_db_version < 30133 && ! defined( 'WP_INSTALLING' ) ) {
		return $term_id;
	}

Since WP_INSTALLING is no more defined when splitting all terms, the test above is passed and thus terms are not splitted (and the '_split_terms' option is updated with wrong values).

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


2 years ago

#39 in reply to: ↑ 37 ; follow-up: @boonebgorges
2 years ago

Replying to Chouby:

I tested 30261.6.diff and there is still a problem when upgrading from an older version (ex: 4.0):

	if ( $current_db_version < 30133 && ! defined( 'WP_INSTALLING' ) ) {
		return $term_id;
	}

Since WP_INSTALLING is no more defined when splitting all terms, the test above is passed and thus terms are not splitted (and the '_split_terms' option is updated with wrong values).

Thanks, Chouby - you are correct. We'll take the db_version check out altogether if we go this route.

@dd32 - I think we probably agree that it's too late to pull out the WP_INSTALLING check in upgrade.php for 4.3.

The major thing that springs to mind here with the retrying, is that the back-compat option _split_terms will become invalid/only partially filled if the process dies part-way (It'll contain the last successful runs data).

Yeah. There are a couple options here:

  1. Save '_split_terms' after each term is split. This makes _split_terms much more reliable - in case of crash, it'd be missing, at most, a single entry. But it slows down the routine by 3-4x. See https://core.trac.wordpress.org/ticket/30261?replyto=37#comment:6
  2. Run the routine in batches of, say, 10 terms. Either (a) fire off a loopback HTTP request, and at the end of that request, if more terms are detected, fire off another request, so it all happens in the background. This will not scale well in the case of network upgrades. Or (b) Keep it hooked to 'admin_init', so that all terms will be split only after a couple pageloads. (We can reduce or eliminate the cap check, to work around your concern about 'update_core'.) The big downside here is that there will be a period - usually short, but it'll vary - where some terms are split and others are not. IMO this is OK, since we're not doing anything in WP (yet) that assumes that all terms are split.
  3. Leave '_split_terms' the way it is. As you note, only a very small number of sites will be affected by failed migrations, and only a small fraction of those will ever need access to '_split_terms'.

#40 in reply to: ↑ 39 ; follow-up: @Chouby
2 years ago

Replying to boonebgorges:

Yeah. There are a couple options here:

Maybe a plugin use case can help to take decisions. In Polylang (a multilingual plugin), terms are grouped in a translation group. I need to update this translation group (which contains one term id per language) when a term is splitted.

In the current stable version (which I wrote with WP 4.2 in mind), I hook to split_shared_term. When term is splitted, I split all terms included in its translation group (I need to do this as a term can not be in several translations groups).

The approach above fails with the current split_all_shared_terms() as terms splitted in Polylang are not taken into account by the core function and I ended up with new terms having no language and no translation group.

Thus I needed to find another approach and work outside the loop. For this I now hook to update_option__split_terms and add_option__split_terms and create new translations groups *only* for terms which have just been splitted (as the _split_terms option may already contain terms which have already been handled since WP 4.2 is out).

  1. Save '_split_terms' after each term is split.

For me that would work only if split_all_shared_terms() checks if the term has not been already splitted (in one of the hooks)

  1. Run the routine in batches of, say, 10 terms.

For me that would work only if the '_split_terms' option is updated only at the end of the whole process (as otherwhise I will have translations inside and outside the "10 terms" and it will be a big mess to deal with).

#41 in reply to: ↑ 40 @boonebgorges
2 years ago

Replying to Chouby:

The approach above fails with the current split_all_shared_terms() as terms splitted in Polylang are not taken into account by the core function and I ended up with new terms having no language and no translation group.

Can you explain this in more detail? When you say "not taken into account", are you referring to the WP_INSTALLING problem, ie that plugins are not loaded? The goal here is to make your 4.2 version - using the 'split_shared_term' hook - work for 4.3 as well.

For this I now hook to update_optionsplit_terms and add_optionsplit_terms and create new translations groups *only* for terms which have just been splitted

This seems very fragile. Am I correct that you're only doing this because of the bug described in this ticket?

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


2 years ago

#43 follow-up: @boonebgorges
2 years ago

Chouby - I apologize - I'm rereading your comment, and looking more closely at Polylang, and I see that you are splitting terms yourself. I'm not sure that I understand the reasoning behind this ("I need to do this as a term can not be in several translations groups"). It seems to me that you could probably *not* do this splitting yourself, and let WordPress handle it during the migration.

In any case,

For me that would work only if split_all_shared_terms() checks if the term has not been already splitted (in one of the hooks)

This does happen - split_all_shared_terms() will not attempt to split a term that has already been split. This is determined by a SQL query that checks for shared terms, *not* by checking the '_split_terms' cached. And even if it *did* try to split it, _split_shared_term() bails with the original $term_id if the term is not, in fact, shared (ie it's already been split). So I think that Polylang shouldn't have to do the update_option__split_terms trick.

But maybe I'm missing something?

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


2 years ago

@boonebgorges
2 years ago

#45 @boonebgorges
2 years ago

Let's try this again. 30261.7.diff simplifies the term splitting process by divorcing it altogether from the database upgrade. When a user with 'edit_posts' visits a wp-admin page ('admin_footer'), wp_batch_split_terms() splits 20 shared terms. This happens over and over again until no more shared terms are found, at which point the 'shared_terms_split' flag is set, and the routine is never run again.

This strategy pretty much eliminates the possibility of out-of-memory or timeout fatal errors, since we're only processing a small number of shared terms on a given pageload. As such, it all but eliminates the possibility of corrupt '_split_terms' back-compat data. It also means we don't need to futz with wp-admin/upgrade.php at all, since plugins are loaded during normal wp-admin pageloads. Fault tolerance is built in: it will keep splitting until it's finished.

Potential downsides:

  • Precedent: I can't think of anywhere in WP where we do anything like this :)
  • Time: I estimate that for 95%+ of all WP installations, there will be fewer than 20 shared terms, so that the splitting will take place on the first pageload after the db upgrade (generally, about.php). But in some cases, it'll take more batches. Depending on wp-admin traffic, there may be some time - hours, days - where not all shared terms are split. IMO, this is not a serious issue, since we're not yet doing anything that requires terms to be split.

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


2 years ago

#47 @dd32
2 years ago

Precedent: I can't think of anywhere in WP where we do anything like this :)

I think there's one main reason why we don't - Race conditions, if two page loads process the term splitting at the same time (say, two authors are logged in) then they'll both be processing term spliting, of the same terms most likely, and the last-one-to-finish updates the _split_terms option.. AFAICT that means we'll still potentially lose some of the data there.

Using Cron is interesting, but is also not guaranteed to run only once, although the locking is better than it used to be.

#48 @peterwilsoncc
2 years ago

30261.8.diff modifies @boonebgorges' work in 30261.7.diff.

  • runs the splitting function on upgrade (now renamed _wp_batch_split_terms & in wp-includes)
  • apart from the scheduling, the batch function is unchanged from Boone's v7 patch
  • _wp_batch_split_terms runs on a cron every 5 minutes (arbitrary number) until there a no more terms to split

@peterwilsoncc
2 years ago

fixes typo in .8.diff

@pento
2 years ago

@pento
2 years ago

#49 follow-ups: @pento
2 years ago

30261.11.diff adds GET_LOCK() / RELEASE_LOCK() calls to _wp_batch_split_terms() in 30261.9.diff, so that only one process is running the routine at a time.

I'm not especially excited about adding the HyperDB-specific code to core, though.

#50 in reply to: ↑ 49 ; follow-up: @ocean90
2 years ago

Replying to pento:

I'm not especially excited about adding the HyperDB-specific code to core, though.

Could this be solved by using the same lock mechanism we're using for updates?
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-upgrader.php?marks=2844-2862,2962#L2827

#51 in reply to: ↑ 43 @Chouby
2 years ago

Replying to boonebgorges:

Chouby - I apologize - I'm rereading your comment, and looking more closely at Polylang, and I see that you are splitting terms yourself. I'm not sure that I understand the reasoning behind this ("I need to do this as a term can not be in several translations groups"). It seems to me that you could probably *not* do this splitting yourself, and let WordPress handle it during the migration.

Let say that you have two shared terms A and B translations of each other. When splitting A, I get A and A1 which would be both translations of B, breaking the reciprocity relationship. B must have only one translation. Thus I split B too.

In any case,

For me that would work only if split_all_shared_terms() checks if the term has not been already splitted (in one of the hooks)

This does happen - split_all_shared_terms() will not attempt to split a term that has already been split. This is determined by a SQL query that checks for shared terms, *not* by checking the '_split_terms' cached.

The problem is that I split terms inside the loop. There is no second check in this function.

And even if it *did* try to split it, _split_shared_term() bails with the original $term_id if the term is not, in fact, shared (ie it's already been split).

I guess that you are referring to this test:

	$shared_tt_count = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM $wpdb->term_taxonomy tt WHERE tt.term_id = %d AND tt.term_taxonomy_id != %d", $term_id, $term_taxonomy_id ) );


	if ( ! $shared_tt_count ) {
		return $term_id;
	}

The problem is that the test never passes in this context as you always count the first term (the one not modified in the splitting process).

In the end, the term is split two times resulting in a big mess in the db.

So I think that Polylang shouldn't have to do the update_option__split_terms trick.

I would much prefer working with the split_shared_term hook (especially if the final choice is to go to batches). But I would need you to check if the term has not been already split inside the loop.


@Chouby
2 years ago

#52 @Chouby
2 years ago

30261.12.diff just adds a check in _split_shared_term() in case a plugin splits terms in a function hooked to split_shared_term.

#53 in reply to: ↑ 49 @Chouby
2 years ago

Replying to pento:

30261.11.diff adds GET_LOCK() / RELEASE_LOCK() calls to _wp_batch_split_terms() in 30261.9.diff, so that only one process is running the routine at a time.

I'm not especially excited about adding the HyperDB-specific code to core, though.

I attempted to test your patch and the process seems to be indefinitely rescheduled. Thus the terms never got split.

I am not competent enough in SQL but I wonder if calls to GET_LOCK() / RELEASE_LOCK() would block plugins to call _split_shared_term() from the split_shared_term hook.

@pento
2 years ago

#54 in reply to: ↑ 50 ; follow-up: @pento
2 years ago

Replying to ocean90:

Could this be solved by using the same lock mechanism we're using for updates?

Good thinking, @ocean90, I forgot about that lock mechanism. 30261.13.diff swaps to locking that way, instead.

Replying to Chouby:

30261.12.diff

Thanks, @Chouby! I've incorporated this into 30261.13.diff, too.

I attempted to test your patch and the process seems to be indefinitely rescheduled.

I think I messed up the lock logic, I was seeing similar behaviour. Could I get you to retest with the new patch?

#55 in reply to: ↑ 54 ; follow-up: @Chouby
2 years ago

Replying to pento:

Could I get you to retest with the new patch?

I just did it (upgrading from a WP 4.0 DB). The terms are correctly split but the plugins are not loaded during the process, making the split_shared_term hook inefficient.

#56 in reply to: ↑ 55 ; follow-up: @ocean90
2 years ago

Replying to Chouby:

but the plugins are not loaded during the process, making the split_shared_term hook inefficient.

Which plugins? Are these checking for DOING_CRON?

#57 in reply to: ↑ 56 @Chouby
2 years ago

Replying to ocean90:

Which plugins? Are these checking for DOING_CRON?

All plugins. During my test, the action split_shared_term is fired while WP_INSTALLING is true and DOING_CRON is not defined.

#58 @boonebgorges
2 years ago

peterwilsoncc - Nice call on the cron. A five-minute interval should pretty much do away with the possibility of race conditions.

Chouby - The reason 13.diff wasn't working was because the first batch was being run during the db upgrade. Instead of calling _wp_batch_split_terms() during the db upgrade, we should be scheduling an event. I've made the change in 30261.14.diff.

pento - The lock mechanism is working well for me. I tried manually causing a race condition, but was thwarted.

I think this is getting close....

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


2 years ago

#60 @Chouby
2 years ago

I tested 30261.14.diff​ and what happens is very strange. Sometimes (rarely) it works exactly as expected, and most often it doesn't.

When it doesn't work 'split_shared_term_batch' seems to be correctly scheduled but is removed from the cron table just after (thus the terms are never split).
I logged the tables saved by _set_cron_array during the upgrade, and noticed that:

  • one call adds 'split_shared_term_batch'
  • the next call removes it but has *two* 'wp_version_check' events (both separated by one day)
Last edited 2 years ago by Chouby (previous) (diff)

#61 @ocean90
2 years ago

30261.14.diff looks good so far. split_shared_term_batch should probably be wp_split_shared_term_batch.

I noticed one race condition: We don't schedule a new event if _wp_batch_split_terms() bails because of a lock situation. Means we never continue splitting terms in such a case.

#62 follow-up: @boonebgorges
2 years ago

Chouby - Thanks for testing. I can't reproduce what you're reporting. It's true that two different 'wp_version_check' hooks are scheduled, but I think this is because one is a recurring event (twicedaily) while the other is sooner due to the TTL. Maybe dd32 can verify. In any case, it's not clear to me how other cron hooks would be wiped out by a call to _set_cron_array(), except as part of a race condition. The only possible place I can imagine this happening is at 'upgrader_process_complete'. Can you give more details about how you're testing the upgrade? dd32, any ideas here?

ocean90 - Good call on both counts. 30261.15.diff changes the event name, and ensures that a new event is scheduled when the lock check fails.

#63 in reply to: ↑ 62 ; follow-up: @Chouby
2 years ago

Replying to boonebgorges:

Can you give more details about how you're testing the upgrade?

Just 3 steps: delete the existing DB from phpMyAdmin, import one which is ready with shared terms (created earlier with WP 4.0) and visit an admin page.

I could reproduce the issue with 30261.15.diff again.

#64 in reply to: ↑ 63 @boonebgorges
2 years ago

Replying to Chouby:

Replying to boonebgorges:

Can you give more details about how you're testing the upgrade?

Just 3 steps: delete the existing DB from phpMyAdmin, import one which is ready with shared terms (created earlier with WP 4.0) and visit an admin page.

I could reproduce the issue with 30261.15.diff again.

Is it possible for you to send me a copy of this database? On Slack or something like that? Are you running any other plugins that might be setting cron jobs?

#65 follow-up: @boonebgorges
2 years ago

I talked privately with Chouby about what he's experiencing. It's a race that occurs when the database upgrade is run at the same time that a wp-cron is in progress. Something like this:

  1. wp-cron.php is hit, and wp_version_check is fired. (wp_version_check was the culprit in Chouby's case, but it could happen with any event, and it'll be liklier with events that require external requests and so take a second or two to complete)
  2. While the cron job is running, the db upgrade is fired in a separate process.
  3. The db upgrade schedules 'wp_split_shared_term_batch' and saves the cron array.
  4. The cron job finishes, and performs cleanup on the $crons array (rescheduling events, etc). This overwrites the cron array from step 3 with the previous value of $crons (with events re- and unscheduled as appropriate).

In Chouby's case, this was happening regularly because he was testing with a database import. The database was old enough that the wp_version_check event was due to run on the first pageload after import. Chouby's first or second pageload was, in fact, the database upgrade, causing the race. This situation is much less likely to occur in the wild, but it's still possible.

It's possible that this is a margin of error that we're OK with. If not, one strategy for mitigating it would be to have a persistent function, maybe hooked to 'admin_init', which would include a check like:

if ( `get_option( 'shared_terms_have_been_split_once_and_for_all' ) && ! wp_next_scheduled( 'wp_batch_split_terms' ) ) {
    wp_schedule_single_event( 'wp_batch_split_terms', time() + MINUTE_IN_SECONDS );
}

This, in turn, would be subject to various race conditions, but wp_batch_split_terms() contains a lock that should prevent problems, and in any case, the function bails when there's nothing to do.

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


2 years ago

@pento
2 years ago

#67 @pento
2 years ago

30261.16.diff implements @boonebgorges' suggestion of the admin_init hook.

The worst case scenario is still that $crons is overwritten by simultaneous processes, but this function can recover from that situation whenever if occurs, as long as someone is visiting wp-admin.

I'm happy with the current state of this. Could another committer please review it for commit? :)

#68 @boonebgorges
2 years ago

16.diff looks good to me. Just ran it through a number of test runs, with various attempts to cause races, but it always recovers nicely. +1 from me for commit.

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


2 years ago

#70 @dd32
2 years ago

30261.16.diff looks good to me! I've been unable to test it thoroughly, but I really can't see anything wrong with it.

There's still the issue of no cron = no split, but I can't imagine that's a significant number of users in todays WordPress world.

#71 @boonebgorges
2 years ago

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

In 33615:

Term splitting routine should be run in a separate process, triggered via wp-cron.

[32814] introduced a routine to split shared terms, which was run during the
regular WP database upgrade. This turned out to be problematic because plugins
are not loaded during the db upgrade (due to WP_INSTALLING), with the result
that plugins were not able to hook into the 'split_shared_term' action during
the bulk split. We work around this limitation by moving the term splitting
routine to a separate process, triggered by a wp-cron hook.

Props boonebgorges, Chouby, peterwilsoncc, pento, dd32.
Fixes #30261.

#72 @boonebgorges
2 years ago

In 33616:

Remove unit test related to pre-4.1 term splitting during wp_update_term().

After [33615], wp_update_term() no longer checks $wp_db_version before
attempting a split. This is because pre-4.1 versions of WordPress must be
allowed to update to 4.3+.

See #30261.

@Chouby
2 years ago

#73 @Chouby
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In 30261.17.diff, I propose to remove the call to wp_suspend_cache_invalidation() as I noticed during my tests that it introduces a conflict with Polylang. That's would not a big deal for Polylang users as it just leaves unused DB entries (so nothing visible to the user), but I have no idea of what kind of other conflict it could introduce in other plugins using the split_shared_term hook.

I guess that this call was introduced for performances reasons. Now that the split is done by batches of 20 terms, it may be less important to save time than avoiding potential conflicts.

@ocean90
2 years ago

#74 follow-up: @ocean90
2 years ago

30261.18.diff: Should we populate finished_splitting_shared_terms for new installs to avoid one schedule?

Edit: Just noticed that the patch is wrong. Needs to be 0 first and changed in upgrade_430().

Last edited 2 years ago by ocean90 (previous) (diff)

#75 in reply to: ↑ 65 @Chouby
2 years ago

Replying to boonebgorges:

It's a race that occurs when the database upgrade is run at the same time that a wp-cron is in progress. Something like this:

  1. wp-cron.php is hit, and wp_version_check is fired. (wp_version_check was the culprit in Chouby's case, but it could happen with any event, and it'll be liklier with events that require external requests and so take a second or two to complete)
  2. While the cron job is running, the db upgrade is fired in a separate process.
  3. The db upgrade schedules 'wp_split_shared_term_batch' and saves the cron array.
  4. The cron job finishes, and performs cleanup on the $crons array (rescheduling events, etc). This overwrites the cron array from step 3 with the previous value of $crons (with events re- and unscheduled as appropriate).

Just for information, there is already a ticket for this wp-cron bug: #13158

#76 in reply to: ↑ 74 ; follow-up: @boonebgorges
2 years ago

Replying to ocean90:

30261.18.diff: Should we populate finished_splitting_shared_terms for new installs to avoid one schedule?

Edit: Just noticed that the patch is wrong. Needs to be 0 first and changed in upgrade_430().

Yes. As you note, it doesn't help us to avoid a schedule (finished_splitting_shared_terms will be set to 1 during the first run of _wp_batch_split_terms()), but it does help to avoid database queries in the absence of a 'notoptions' cache. This was in an earlier version of the patch, but got lost in translation.

Replying to Chouby:

In 30261.17.diff​, I propose to remove the call to wp_suspend_cache_invalidation() as I noticed
during my tests that it introduces a conflict with Polylang. That's would not a big deal for
Polylang users as it just leaves unused DB entries (so nothing visible to the user), but I have
no idea of what kind of other conflict it could introduce in other plugins using the
split_shared_term hook.

I guess that this call was introduced for performances reasons. Now that the split is done by
batches of 20 terms, it may be less important to save time than avoiding potential conflicts.

Yes, it was introduced for performance reasons, and yes, it's less relevant now that it's done in batches. (Rebuilding taxonomy hierarchies can be very resource-intensive in certain cases.) Before removing the invalidation suspension, can you give a few more details about how Polylang is being affected? I want to make sure we're not papering over a larger bug.

Last edited 2 years ago by boonebgorges (previous) (diff)

#77 in reply to: ↑ 76 ; follow-up: @Chouby
2 years ago

Replying to boonebgorges:

Before removing the invalidation suspension, can you give a few more details about how Polylang is being affected? I want to make sure we're not papering over a larger bug.

  1. One step in the loop creates a term A and associates it to an object (one of the split shared term).
  2. In another step, the association is removed and then I call get_term() to get the term A. The function checks for the term_count which is 1 instead of 0 and thus does not remove the term A when it should.

Obviously this process doesn't make sense as a whole, but inside the function hooked to split_shared_term, I use generic functions of Polylang which were not specifically designed for the term split.

#78 in reply to: ↑ 77 @boonebgorges
2 years ago

Replying to Chouby:

Replying to boonebgorges:

Before removing the invalidation suspension, can you give a few more details about how Polylang is being affected? I want to make sure we're not papering over a larger bug.

  1. One step in the loop creates a term A and associates it to an object (one of the split shared term).
  2. In another step, the association is removed and then I call get_term() to get the term A. The function checks for the term_count which is 1 instead of 0 and thus does not remove the term A when it should.

Obviously this process doesn't make sense as a whole, but inside the function hooked to split_shared_term, I use generic functions of Polylang which were not specifically designed for the term split.

Gotcha - thanks for the details. I'm OK with not suspending cache invalidation, but at the risk of being overly cautious, I think we should then reduce the batch size. In 30261.19.diff it's 10 instead of 20.

#79 @pento
2 years ago

I'm fine with 30261.19.diff being committed.

#80 @pento
2 years ago

  • Keywords commit added; needs-testing removed

#81 @boonebgorges
2 years ago

In 33619:

Don't suspend cache invalidation when running term splitting batch routine.

wp_suspend_cache_invalidation() was originally added to increase performance,
but the switch to batch processing in [33615] mitigates serious performance
concerns.

As a precaution against timeouts, the batch size has been reduced from 20 to 10.

Props Chouby.
See #30261.

#82 follow-up: @obenland
2 years ago

Not fixed?

#83 in reply to: ↑ 82 @boonebgorges
2 years ago

  • Keywords commit removed

Replying to obenland:

Not fixed?

Sorry - almost. We need to properly set a default value for the 'finished_splitting_shared_terms' option, to prevent 'notoptions' database queries before the routine is finished. 30261.20.diff needs review before commit.

#84 follow-up: @pento
2 years ago

I'm wondering if it's worth having the default value as 1, and set it to 0 in pre_schema_upgrade()? This will avoid unnecessarily running the term split function on new installs.

#85 in reply to: ↑ 84 ; follow-up: @boonebgorges
2 years ago

Replying to pento:

I'm wondering if it's worth having the default value as 1, and set it to 0 in pre_schema_upgrade()? This will avoid unnecessarily running the term split function on new installs.

Good idea. Unless I'm missing something, upgrade_430() should be a fine place to do this. See 30261.21.diff.

#86 @boonebgorges
2 years ago

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

In 33621:

Don't run term-splitting routine on new installations.

Props pento.
Fixes #30261.

#87 in reply to: ↑ 85 @pento
2 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to boonebgorges:

Unless I'm missing something, upgrade_430() should be a fine place to do this. See 30261.21.diff.

It needs to be in pre_schema_upgrade().

upgrade_430() is called from upgrade_all(), which is called from wp_upgrade() after pre_schema_upgrade().

Before upgrade_all() calls the upgrade_*() functions, it calls populate_options(), which will pre-fill with 'finished_splitting_shared_terms' => 1.

#88 @pento
2 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Nevermind, I re-opened prematurely.

finished_splitting_shared_terms will be overwritten to 0 by upgrade_430(). The worst case is that we try to schedule the cron between populate_options() and upgrade_430(), but fail because finished_splitting_shared_terms is 1. We'll schedule the cron properly next wp-admin page load.

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


2 years ago

#90 @dd32
2 years ago

In 33646:

Term Splitting: Fix a reversal of parameters to wp_schedule_single_event() introduced in [33621].
The existing invalid cron entries will not be purged automatically (as the 'timestamp' is never matched) so we do this ourselves.

Props mechter for noticing!
See #30261.
Fixes #33423 for trunk.

#91 @dd32
2 years ago

In 33647:

Term Splitting: Fix a reversal of parameters to wp_schedule_single_event() introduced in [33621].
The existing invalid cron entries will not be purged automatically (as the 'timestamp' is never matched) so we do this ourselves.

Merges [33646] to the 4.3 branch.
Props mechter for noticing!
See #30261.
Fixes #33423 for the 4.3 branch.

Note: See TracTickets for help on using tickets.