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

[leewillis77]

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.

[leewillis77]

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.

[leewillis77]

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.

[leewillis77]

Revised reproduction details

[leewillis77]

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.

[leewillis77]

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.

[ryan]

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.

[ryan]

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.

[ryan]

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.

[leewillis77]

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?

[nacin]

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.

[leewillis77]

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]

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

[ryan]

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

[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

[JustinSainton]

@leewillis77 - see ​http://core.trac.wordpress.org/browser/tags/3.3.1/wp-includes/query.php#L1968.

[leewillis77]

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

[duck_]

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.

[leewillis77]

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?

[ryan]

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

[leewillis77]

Patch to resolve issues that duck_ raised.

[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".

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?

[duck_]

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.

[duck_]

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.

[markoheijnen]

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

[ryan]

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]

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]

Formatting tweaks

[ryan]

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 passed to the clean_post_cache action.

[leewillis77]

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.

[nacin]

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

[ryan]

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

[leewillis77]

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.

[ryan]

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.

[ryan]

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

[leewillis77]

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.

[ryan]

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.

[ryan]

The relevant cache tests are here: ​http://unit-tests.trac.wordpress.org/browser/wp-testcase/test_includes_post.php#L75

[ryan]

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

[ryan]

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