Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#36251 closed defect (bug) (fixed)

Allowed memory size exhausted on wp_update_term

Reported by: rahalaboulfeth's profile rahal.aboulfeth Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.9
Component: Taxonomy Keywords:
Focuses: performance Cc:

Description

Hello,

I have a category with a lot of posts ( more than 70000 ) , and I can't update it from the dashboard because of a Allowed memory exhaustion error.

After reviewing wp_update_term code, I noticed this section line 3282 in taxonomy.php :

<?php
        // Clean the relationship caches for all object types using this term.
        $objects = $wpdb->get_col( $wpdb->prepare( "SELECT object_id FROM $wpdb->term_relationships WHERE term_taxonomy_id = %d", $tt_id ) );
        $tax_object = get_taxonomy( $taxonomy );
        foreach ( $tax_object->object_type as $object_type ) {
                clean_object_term_cache( $objects, $object_type );
        }


In my case objects is an array with more than 70000 rows, and this is probably what caused the memory exhaustion. I commented this section and the update started working again.

Now, I don't really get why we should clean all those caches.
I think the same behavior is also present in wp_delete_term.

Thank you

Attachments (1)

36251.diff (4.8 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (7)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Taxonomy
  • Summary changed from Allowed memory size exhausted on up_update_term to Allowed memory size exhausted on wp_update_term

#2 @rahal.aboulfeth
8 years ago

Hello,
Any ideas how to fix this?

#3 @boonebgorges
8 years ago

  • Focuses administration removed
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 4.4.2 to 3.9

It's necessary to bust the caches on term deletion. If a post has terms a and b, and b is deleted, you have to remove the association with b from the cache. I don't think there's a way around this.

The cache busting in wp_update_term() was introduced as part of #22526. The reason why it's needed is the {$taxonomy}_relationships cache contains arrays describing the entire term object, rather than simple term IDs. I think it was probably originally built this way because we didn't have decent cache support for single terms. Now we do, so we can probably explore changing how the object relationship cache works. If it stores only term IDs, and fetches the terms themselves from their own caches, it should no longer be necessary to call clean_object_term_cache() when updating a term.

@boonebgorges
8 years ago

#4 @boonebgorges
8 years ago

  • Milestone changed from Future Release to 4.6

36251.diff is a first attempt at fixing the problem. Notes:

  • Anywhere where the {$taxonomy}_relationships cache is set, use an array of term IDs rather than full term data.
  • When term data is pulled from the cache (get_object_term_cache()), convert term IDs to term data arrays. There is a bit of juggling involved. wp_cache_get() gives [1, 2, 3]. In order to get terms 1, 2, and 3 from the cache, we use get_term( 1 ) etc, which returns a WP_Term object. But functions calling get_object_term_cache() expect an array rather than WP_Term, so we return the $data property from the term objects, and let the functions that call get_object_term_cache() convert those arrays back into WP_Term objects. It's a bit clunky, but required for backward compatibility.
  • Some unit tests are no longer relevant, so they've been updated for the new technique.

This seems to me a much improved strategy over what's currently in place. By reducing redundancy in the term data we cache (since things like name, etc are only cached with the individual object), we reduce the size of the cache, and fix bugs like #22526 along the way.

The one remaining compatibility issue I can think of is that there may be plugins calling wp_cache_set( $object_id, "{$taxonomy}_relationships" ) directly, rather than update_object_term_cache(). If plugins manually put term arrays rather than term IDs into the cache, things will break. Generally, we don't go out of our way to maintain backward compatibility with the structure of the data we keep in our internal caches, so maybe this is not a concern. If it is, get_object_term_cache() could include a check to see whether the cached data is in the legacy format, and convert as necessary.

Feedback welcome.

#5 @boonebgorges
8 years ago

A refined version of 36251.diff was committed in [37573]. See #36814.

It appears that we no longer need to be invalidating object relationships caches on term update. Let's tear it out.

#6 @boonebgorges
8 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 37593:

Don't clear object relationship caches on term update.

Since [37573], object relationship caches ({$taxonomy}_relationships)
contain term IDs rather than term objects. See #36814. As such, it's no longer
necessary to clear these caches when a term is updated; none of the data that's
changed on update (name, description, count, etc) is stored in the relationship
cache.

Fixes #36251.

Note: See TracTickets for help on using tickets.