Make WordPress Core

Opened 8 years ago

Last modified 2 years ago

#37762 assigned defect (bug)

cache_results parameter doesn't prevent queried posts from being added to cache

Reported by: dang-vu's profile Dang Vu Owned by: boonebgorges's profile boonebgorges
Milestone: Future Release Priority: normal
Severity: normal Version: 4.6
Component: Query Keywords: needs-patch dev-feedback
Focuses: performance Cc:

Description

Even when cache_results is set to false, the queried posts in WP_Query are still mapped to get_post() which will always add post instance to cache.

For more info: http://wordpress.stackexchange.com/questions/236653/how-to-prevent-queried-posts-from-being-added-to-cache/236659.

Change History (10)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Query

This ticket was mentioned in Slack in #core by mark-k. View the logs.


8 years ago

#3 follow-up: @achbed
8 years ago

What's interesting is that the individual posts are loaded and cached to the object cache individually, but the combined query result is what is controlled via the cache_results parameter. This seems to meet the defined parameter intent because it's not caching the full result of the query. If the posts themselves change enough that you can't use object caching I'd be truly surprised.

If you want to prevent the posts themselves from caching, that appears to be a different request and intent. Maybe an additional cache_posts parameter to control that piece of the caching puzzle?

We should update the docs at the very least to explain this behavior.

#4 in reply to: ↑ 3 @Dang Vu
8 years ago

Replying to achbed:

What's interesting is that the individual posts are loaded and cached to the object cache individually, but the combined query result is what is controlled via the cache_results parameter. This seems to meet the defined parameter intent because it's not caching the full result of the query. If the posts themselves change enough that you can't use object caching I'd be truly surprised.

If you want to prevent the posts themselves from caching, that appears to be a different request and intent. Maybe an additional cache_posts parameter to control that piece of the caching puzzle?

We should update the docs at the very least to explain this behavior.

You're right! The usage of the cache_results parameter is unclear. A ticket for cache_posts parameter looks promising.
Honestly, I don't understand why WordPress needs to add queried posts to both the WP_Query::posts and the cache since they only exist in a single page load.

#5 follow-ups: @boonebgorges
8 years ago

  • Keywords reporter-feedback added
  • Owner set to boonebgorges
  • Status changed from new to assigned

Hi @Dang Vu - Thanks for the ticket.

Honestly, I don't understand why WordPress needs to add queried posts to both the WP_Query::posts and the cache since they only exist in a single page load.

If you are running a persistent object cache like Memcached, then the cached post objects are available across page loads. Even within a single pageload and without a persistent cache, other functions in WP fetch data from the 'posts' cache group. WP_Query::posts is not intended to be a cache, but is a list of query results.

cache_results is fairly poorly named. Introduced in [14310], it really means "populate all caches related to query results". This includes the 'posts' cache as well as the taxonomy and metadata caches. See update_post_caches(). In [14665], cache_results was set to false when a persistent object cache is in use, because the repeated (unnecessary) round trips to Memcached were causing performance issues; see #12611. The posts array in WP_Query began to touch the 'posts' cache in [21559], which made cache_results more ambiguous: it now determines whether metadata and taxonomy should automatically be loaded into the cache, rather than the post objects. And this latter fact is confused by the fact that we have separate update_post_meta_cache and update_post_term_cache params.

From this, I conclude that cache_results doesn't really do what it was meant to do. If anything, it should be removed altogether.

What is the reason for wanting to prevent post objects from being loaded into the cache? Is it a performance issue like described in #12611? If so, I wonder if we could mostly mitigate the issue with cache multi-get and multi-set, which would dramatically reduce the number of round-trips necessary to load data into the cache; see #20875.

#6 in reply to: ↑ 5 @achbed
8 years ago

Replying to boonebgorges:

From this, I conclude that cache_results doesn't really do what it was meant to do. If anything, it should be removed altogether.

Are there any backwards-compatible reasons to not remove/ignore the parameter? The only thing I can think of is a slight performance hit if we're now no longer updating the metadata and/or taxonomy caches. I think that's acceptable.

What is the reason for wanting to prevent post objects from being loaded into the cache? Is it a performance issue like described in #12611? If so, I wonder if we could mostly mitigate the issue with cache multi-get and multi-set, which would dramatically reduce the number of round-trips necessary to load data into the cache; see #20875.

The only reason I can think of is if the post includes some data that changes very often (for example - some plugin is attaching random or API data to a post at load time). In that case they're probably doing_it_wrong() and should be discouraged anyway so that we can use the cache properly.

#7 @boonebgorges
8 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Are there any backwards-compatible reasons to not remove/ignore the parameter? The only thing I can think of is a slight performance hit if we're now no longer updating the metadata and/or taxonomy caches. I think that's acceptable.

Currently, cache_results (when used properly alongside update_post_meta_cache and update_post_term_cache) prevents at least two additional cache roundtrips for each fetched post, and possibly many more in the case of lots of custom taxonomies. Removing the option altogether could cause serious performance problems in certain cases. Once #20875 is addressed, I think we can revisit the removal of cache_results.

The only reason I can think of is if the post includes some data that changes very often (for example - some plugin is attaching random or API data to a post at load time). In that case they're probably doing_it_wrong() and should be discouraged anyway so that we can use the cache properly.

Yeah, filtering the post data pre-cache could cause problems, but I think that in this case the solution is to filter *post* cache. The frequency of updates is an argument *for* better cache leverage, not against it; if there are inconsistency issues in these cases, it's likely due to an invalidation bug, rather than the fact that the cache is being used in the first place.

I'm going to move this ticket to Future Release so it can be reassessed post-#20875.

#8 in reply to: ↑ 5 @Dang Vu
8 years ago

Last edited 8 years ago by Dang Vu (previous) (diff)

#9 @desrosj
5 years ago

  • Milestone set to Future Release

#10 @mukesh27
2 years ago

  • Keywords needs-patch dev-feedback added

Hi there!

This issue still happens if we use the ticket author's suggested code snippet.

<?php
echo count($wp_object_cache->cache['posts']);

$query = new \WP_Query([
  'post_type' => 'post',
  'cache_results' => false,
  'posts_per_page' => 5,
  'ignore_sticky_posts' => true
]);

echo '<br>' . count($wp_object_cache->cache['posts']);

dev-feedback added so we get more eyes on this issue and someone from core time will tackle this.

Note: See TracTickets for help on using tickets.