Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#19690 closed defect (bug) (fixed)

wp_suspend_cache_additions() doesn't block term_relationships being added to cache

Reported by: leewillis77's profile leewillis77 Owned by: ryan's profile ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

  1. Register a custom taxonomy, and create one or more posts associated with this taxonomy
  2. Place the attached code in your theme's functions.php [Tweak the args to get_posts accordingly to retrieve the posts created in (1)]
  3. Load your blog and inspect your error log.

The output should confirm that while wp_suspend_cache_additions() blocks most cache additions, cache entries are still added for {$taxonomy}_relationships. E.g. in my case, the output is:

[29-Dec-2011 22:49:06] --------------
[29-Dec-2011 22:49:06] options - 6 items
[29-Dec-2011 22:49:06] default - 1 items
[29-Dec-2011 22:49:06] users - 1 items
[29-Dec-2011 22:49:06] userlogins - 1 items
[29-Dec-2011 22:49:06] useremail - 1 items
[29-Dec-2011 22:49:06] userslugs - 1 items
[29-Dec-2011 22:49:06] user_meta - 1 items
[29-Dec-2011 22:49:06] --------------
[29-Dec-2011 22:49:06] --------------
[29-Dec-2011 22:49:06] options - 6 items
[29-Dec-2011 22:49:06] default - 1 items
[29-Dec-2011 22:49:06] users - 1 items
[29-Dec-2011 22:49:06] userlogins - 1 items
[29-Dec-2011 22:49:06] useremail - 1 items
[29-Dec-2011 22:49:06] userslugs - 1 items
[29-Dec-2011 22:49:06] user_meta - 1 items
[29-Dec-2011 22:49:06] wpsc_product_category_relationships - 50 items
[29-Dec-2011 22:49:06] product_tag_relationships - 50 items
[29-Dec-2011 22:49:06] wpsc-variation_relationships - 50 items
[29-Dec-2011 22:49:06] wpec_product_option_relationships - 50 items
[29-Dec-2011 22:49:06] --------------

As you can see entries have been added to the cache for the various {taxonomy}_relationships.

This appears to be either because the call to wp_cache_set inside update_object_term_cache() in taxonomy.php should be wp_cache_add - and/or wp_cache_set should check the value of wp_suspend_cache_additions() before adding to the cache.

Attachments (8)

test.php (656 bytes) - added by leewillis77 13 years ago.
19690.diff (404 bytes) - added by leewillis77 13 years ago.
Given that update_object_term_cache declares in its documentation that it "Will only update the cache for terms not already cached.", the attached patch amends it to call wp_cache_add instead of wp_cache_set. On reading the code this appears to make sense.
test2.php (591 bytes) - added by leewillis77 13 years ago.
Revised reproduction details
19690.2.diff (1.8 KB) - added by ryan 13 years ago.
19690.21.diff (4.1 KB) - added by leewillis77 13 years ago.
Patch to resolve issues that duck_ raised.
19690.3.diff (7.5 KB) - added by ryan 13 years ago.
19690.4.diff (7.7 KB) - added by ryan 13 years ago.
Formatting tweaks
19690.5.diff (8.0 KB) - added by leewillis77 13 years ago.

Download all attachments as: .zip

Change History (47)

@leewillis77
13 years ago

@leewillis77
13 years ago

Given that update_object_term_cache declares in its documentation that it "Will only update the cache for terms not already cached.", the attached patch amends it to call wp_cache_add instead of wp_cache_set. On reading the code this appears to make sense.

#1 @leewillis77
13 years ago

On reflection - I suspect the title of this bug is inaccurate - I believe this affects all taxonomies, not just custom ones.

My initial observation that only custom taxonomies had their relationships inserted into the cache was I believe because the posts I was querying were limited to ones which only had custom taxonomies.

I'll do some testing to confirm when I'm back at a suitable location, and update either way. but just wanted to note it here on the off-chance someone looks at this in the meantime.

#2 @husobj
13 years ago

  • Cc ben@… added

@leewillis77
13 years ago

Revised reproduction details

#3 @leewillis77
13 years ago

  • Summary changed from wp_suspend_cache_additions() doesn't block term_relationships with custom taxonomies to wp_suspend_cache_additions() doesn't block term_relationships being added to cache

Confirmed that this affects all taxonomies, not just custom taxonomies. Steps to reproduce therefore are just:

  1. Place the attached code (test2.php) in your theme's functions.php
  2. Load your blog and then inspect your error log.

The results should show that entries have been added to the cache for category_relationships, post_tag_relationships, and post_format_relationships despite the call to wp_suspend_cache_addition. Proposed patch is still the same - just clarifying the affected taxonomies.

#4 @JustinSainton
13 years ago

  • Cc JustinSainton added

#5 @SergeyBiryukov
13 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.4

#6 @leewillis77
13 years ago

Thanks for the review - is there anything we can do to bring this in before 3.4? This (together with #19708) are preventing some pretty significant performance wins in a number of projects, and it'd be great to get them in core sooner rather than later.

Happy to do whatever's needed to help with that...

Thanks again.

#7 @ryan
13 years ago

Why is wp_suspend_cache_additions() being used? Currently, its role is limited to reducing cache activity during imports, where we know we are doing strict adds for new data. It is not safe to use in most situations.

#8 @ryan
13 years ago

That's been using wp_cache_set() since it was introduced in [6243]. It could probably change to add, but plugin use of update_object_term_cache() would have to be investigated.

BTW, get_posts() has a cache_results argument specifically to disable updating these caches.

#9 @ryan
13 years ago

Core calls update_object_term_cache() strictly to prime caches, not propagate changes. Any plugin using this function to propagate changes to a post's relationships to the cache must be doing very naughty things like bare SQL queries. In general, we prefer the delete/add model to the set model. So, the use of wp_cache_set() instead of wp_cache_add() here seems to be an unintended artifact.

#10 follow-up: @leewillis77
13 years ago

Hi Ryan,

Thanks for your comments. The usage is in plugins that wish to iterate over all posts (For some reason). The specific use case that caused this is generating an XML feed of product information from e-Commerce stores for Google Merchant Centre. In this context you want to iterate over all posts, but there's no specific advantage in adding those posts (Or related information) to the cache.

Background here:
http://code.google.com/p/wp-e-commerce/issues/detail?id=885

My reading of the code in update_object_term_cache() is that the final bit will only be reached where the item doesn't already exist in the cache anyway - so changing to _add from _set should be realistically functionally equivalent?

#11 in reply to: ↑ 10 @nacin
13 years ago

Replying to leewillis77:

In this context you want to iterate over all posts, but there's no specific advantage in adding those posts (Or related information) to the cache.

Background here:
http://code.google.com/p/wp-e-commerce/issues/detail?id=885

As Ryan points out, the plugin should be setting cache_results to false for WP_Query.

My reading of the code in update_object_term_cache() is that the final bit will only be reached where the item doesn't already exist in the cache anyway - so changing to _add from _set should be realistically functionally equivalent?

Yes.

#12 @leewillis77
13 years ago

I wasn't aware of a cache_results option to get_posts, it's not in the codex, and a quick Google didn't return any obvious results - can you post a reference so I can compare that behaviour to this?

@ryan
13 years ago

#13 @ryan
13 years ago

I found some *_relationships cache bugs while I was in there. Updated unit tests: http://unit-tests.trac.wordpress.org/changeset/496

#14 @ryan
13 years ago

get_posts()/WP_Query has three cache control flags: cache_results, update_post_term_cache, and update_post_meta_cache. cache_results is a master flag that will turn off the other two. If an object cache backend such as memcached is used, the flags are set to false by default since there is no need to update the cache every page load when a persistent cache exists.

http://core.trac.wordpress.org/browser/trunk/wp-includes/query.php#L1967

Last edited 13 years ago by ryan (previous) (diff)

#15 follow-up: @ryan
13 years ago

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

In [19729]:

Use wp_cache_add() instead of wp_cache_set() when priming the object term cache in update_object_term_cache(). Pass the real post_type to clean_object_term_cache() instead of hard-coding post. Call clean_object_term_cache() from clean_bookmark_cache(). Props leewillis77. fixes #19690

#17 @leewillis77
13 years ago

Thanks Justin/Ryan/Andrew - I've updated the codex with the info on cache_results for others' reference.

#18 in reply to: ↑ 15 @duck_
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to ryan:

In [19729]:

Use wp_cache_add() instead of wp_cache_set() when priming the object term cache in update_object_term_cache(). Pass the real post_type to clean_object_term_cache() instead of hard-coding post. Call clean_object_term_cache() from clean_bookmark_cache(). Props leewillis77. fixes #19690

This causes a notice in clean_post_cache() when it's called from wp_delete_post() and the post being deleted isn't in the cache. This is because get_post() will not return an object as the post no longer exists.

PHP Notice:  Trying to get property of non-object in wp-includes/post.php on line 4368

This is quite hard to reproduce under normal conditions as it is often masked by the post being in the cache at the get_post() call. Artificial reproduction is easy by calling wp_suspend_cache_addition( true ) before wp_delete_post(). It also causes an error in the unit tests when WP_DEBUG is enabled.

#19 @leewillis77
13 years ago

It seems that the changes to post.php in [19729] are the result of comment #13. They're not required to fix the original bug reported here as far as I can tell, or see.

So - if we need to revert anything, I don't think the other changes need to be reverted, they stand alone.

Providing an alternative patch seems to be beyond my knowledge of the WP inner workings I'm afraid, but it seems like clean_post_cache needs an additional second argument for $post_type since we can't guarantee that it can be found out at that point in the execution?

#20 @ryan
13 years ago

duck_ and I discussed this. Passing the type to clean_post_cache() should suffice.

@leewillis77
13 years ago

Patch to resolve issues that duck_ raised.

#21 follow-ups: @leewillis77
13 years ago

The attached patch implements that additional option, and updates all callers in core to pass the additional parameter. The additional parameter defaults to "post".

For bonus points we could probably deprecate clean_page_cache, by adding an additional

do_action('clean_'.$post_type.'_cache', $id);

inside clean_post_cache(), but not sure if that's desirable?

#22 in reply to: ↑ 21 @duck_
13 years ago

Replying to leewillis77:

For bonus points we could probably deprecate clean_page_cache, by adding an additional

do_action('clean_'.$post_type.'_cache', $id);

inside clean_post_cache(), but not sure if that's desirable?

That's not a bad idea, but the patch as it is cannot drop the special handling for pages because clean_page_cache() clears the "all_page_ids" and "get_pages" keys. We would also need to move cleaning of those into clean_post_cache() inside an if-statement, or hook into the proposed action with a callback.

#23 in reply to: ↑ 21 @duck_
13 years ago

Replying to leewillis77:

The attached patch implements that additional option, and updates all callers in core to pass the additional parameter. The additional parameter defaults to "post".

Another issue I just noticed was the recursion in clean_post_cache(). $cid is an integer, not an object, so $cid->post_type is incorrect.

#24 @markoheijnen
13 years ago

  • Cc marko@… added

Why this way. why not move clean_page_cache/clean_post_cache above $wpdb->delete() ?

#25 @ryan
13 years ago

We usually clean after the delete in case a hook gets fired in between the cache clear and the delete that fetches the post and restores the cache.

@ryan
13 years ago

#26 @ryan
13 years ago

19690.3.diff builds on 19690.21.diff.

  • Deprecates clean_page_cache() and update_post_cache()
  • Drops do_action('clean_page_cache', $id)
  • Clears all_page_ids and get_pages in clean_post_cache()
  • Assumes children are the same post_type in the child loop

@ryan
13 years ago

Formatting tweaks

#27 @ryan
13 years ago

Perhaps leave the clean_page_cache action in for now. Need to audit plugins to see how often it is used. post_type should be pased to the clean_post_cache action.

Version 0, edited 13 years ago by ryan (next)

@leewillis77
13 years ago

#28 @leewillis77
13 years ago

Hi Ryan,

Thanks for catching my dumb error about children being ints not post objects.

I was already prepping an updated patch however, as I don't think your assumption about child post_type being equal to parent post type is sensible (At least it's not true for a single one of my dev installs). The attached patch grabs the child's post type from the DB instead of assuming it matches the parents.

It also includes your comments from #27 about leaving clean_page_cache in - although I've done it as a more generic clean_$post_type_cache action which preserves the functionality for clean_page_cache as well as offering more flexibility for other post types should it be required.

#29 @nacin
13 years ago

Would it make sense to pass the full post object to clean_post_cache() rather than separately passing the ID and the post type?

I ask because the function previously called get_post() on the $id, which means plugins may have already been passing in objects (and it would have worked just fine). Could cause some problems...

#30 @ryan
13 years ago

Since we're PHP5 now and post objects pass by ref, no objections.

#31 @leewillis77
13 years ago

The call to get_post was introduced in this bug and hasn't hit a stable release.

We can't put that back in as the post isn't guaranteed to exist anywhere we could retrieve it from (DB or cache - See comment #18), so we shouldn't put it back in.

#32 @ryan
13 years ago

We must make clean_post_cache() sane with regard to post type. It wasn't before, and we have unit tests now that demonstrate the cache corruption.

#33 @ryan
13 years ago

But the point that get_post() is new to 3.4 is taken. That alone is not reason enough to switch to passing objects.

#34 @leewillis77
13 years ago

Hi Ryan,

Are the tests still failing with the patch in 19690.5.diff? If so, can you confirm exactly which tests. I ran php wp-test.php -t WPTestIncludesPost without any failures, but if there are others I should be running, happy to investigate and update the patch.

#35 @ryan
13 years ago

I don't think we need clean_$post_type_cache. do_action( 'clean_post_cache', $id, $post_type ); should be sufficient. We no longer use the post type in any cache keys so it feels rights to standardize on just post for the action.

#37 @ryan
13 years ago

In [20423]:

Pass post_type to clean_post_cache() instead of attempting to fetch a post object since the post may have been deleted.

Props leewillis77
see #19690

#38 @ryan
13 years ago

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

#39 @ryan
13 years ago

Similar user cache issues: #19500 #20460. If we decide to start passing objects, let's handle posts in a new ticket.

Note: See TracTickets for help on using tickets.