Make WordPress Core

#57163 closed defect (bug) (fixed)

_prime_post_caches() doesn't account for primed posts without primed meta/terms

Reported by: ocean90's profile ocean90 Owned by: ocean90's profile ocean90
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Query Keywords: has-patch commit has-unit-tests fixed-major
Focuses: performance Cc:

Description

_prime_post_caches() tries to be smart about whether posts cache needs to be updated by first checking if IDs aren't already cached. Unfortunately this becomes an issue if the post itself is already cached but not the meta/terms. In this case _get_non_cached_ids() returns an empty array and thus update_post_caches() isn't called any longer.

I notices this first by looking into update_post_thumbnail_cache() since it wasn't working as expected.

  • It calls get_post_thumbnail_id() which calls get_post() and thus primes the cache in the posts group
  • Because of that _prime_post_caches() basically does nothing anymore since the post is already cached
  • Result: No meta cache is primed

But then I also found another case which was due to the changes in [54352] for 6.1. update_post_caches() was replaced with _prime_post_caches() causing the same issue as mentioned above. If a post is already cached but meta/terms not, the cache won't be updated. So this part is actually a regression in 6.1.

I'm not sure if we can update _prime_post_caches() to also check if meta/terms caches are already set or if should just continue using update_post_caches().

Change History (24)

#1 follow-up: @peterwilsoncc
19 months ago

I think this is a bug with _prime_post_caches().

For post meta, a separate check can be done for _get_non_cached_ids( $ids, 'post_meta' );.

Post terms would be more complicated as the group name differs for each taxonomy: "{$taxonomy}_relationships". update_object_term_cache() already handles this so if the function is called to warm caches, then it might be possible to call without wrapping it in a _get_non_cached_ids() check.

This ticket was mentioned in PR #3658 on WordPress/wordpress-develop by @peterwilsoncc.


19 months ago
#2

  • Keywords has-patch added; needs-patch removed

#3 in reply to: ↑ 1 @ocean90
19 months ago

Replying to peterwilsoncc:

I think this is a bug with _prime_post_caches().

I tend to agree here, especially since the function is no longer considered to be private.

#4 @spacedmonkey
19 months ago

The issue with all the _prime functions, is that is just guesses that the if the main object is primed not to both to prime the meta / terms. This data should treated as different types and we should ensure it primed always.

@ocean90 commented on PR #3658:


19 months ago
#5

Edit: I suspect Test_Query_CacheResults::test_author_cache_warmed_by_the_loop fails due to this very issue. The post cache is warmed by wp_insert_post() when created but its term and meta data cache is not.

Yes, that makes sense. We should probably start documenting what type of queries are expected instead of only having a "random" number in assets call.

@spacedmonkey commented on PR #3658:


19 months ago
#7

I have created an alternative version of this PR https://github.com/WordPress/wordpress-develop/pull/3662. This just removes the usage of update_post_caches.

#8 follow-ups: @ocean90
19 months ago

_prime_comment_caches(), _prime_term_caches(), and _prime_site_caches() are having the same issue. So whatever solution we'll use here should also get applied to them. Will create a follow-up ticket.

#9 in reply to: ↑ 8 @spacedmonkey
19 months ago

Replying to ocean90:

_prime_comment_caches(), _prime_term_caches(), and _prime_site_caches() are having the same issue. So whatever solution we'll use here should also get applied to them. Will create a follow-up ticket.

+1 to this. We just have to ensure that, we do not break lazy loading of term / comment meta at the same time.

@peterwilsoncc commented on PR #3658:


19 months ago
#10

In addition to my suggestions earlier, I've pushed:

  • added Tests_Post_PrimePostCaches to the cache group too
  • new test to ensure lazy loading term meta works as expected (this probably falls under test improvements for the purpose of commits.
  • self merged my suggestions earlier.

I think this is good to go in but as it's a mix of my, @ocean90's and @spacedmonkey's code we should probably approve each others contributions prior to commit.

#11 @peterwilsoncc
19 months ago

  • Keywords commit has-unit-tests added

The linked pull request is a group effort and has committer approvals of each other's code.

The commit should probably include a "see #56793" tag as it adds an additional test for query caching.

I'm marking this as commit ready.

@spacedmonkey commented on PR #3658:


19 months ago
#12

@peterwilsoncc Should we change _prime_comment_caches(), _prime_term_caches(), and _prime_site_caches() in this PR as well. They should all work the same way, IMO.

@ocean90 commented on PR #3658:


19 months ago
#13

@spacedmonkey We'll change them but in a new ticket/PR for 6.2 since this isn't regression unlike this one.

@spacedmonkey commented on PR #3658:


19 months ago
#14

@spacedmonkey We'll change them but in a new ticket/PR for 6.2 since this isn't regression unlike this one.

Let's do this in another ticket in 6.2.

#15 @ocean90
19 months ago

  • Summary changed from Don't use _prime_post_caches() if meta/terms cache should be updated to _prime_post_caches() doesn't account for primed posts without primed meta/terms

#16 @ocean90
19 months ago

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

In 54892:

Query: Account for primed post caches without primed post meta/term caches.

In [54352] update_post_caches() was replaced by _prime_post_caches() to reduce excessive object cache calls. That's because _prime_post_caches() checks first if post IDs aren't already cached. Unfortunately this becomes an issue if a post itself is cached but not the meta/terms.
To fix this regression, _prime_post_caches() now always calls update_postmeta_cache() and update_object_term_cache() depending on the arguments passed to it. Both functions internally check whether IDs are already cached so the fix from [54352] remains in place.

Props peterwilsoncc, spacedmonkey, ocean90.
Fixes #57163.

#17 @ocean90
19 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for backport to 6.1

#18 @ocean90
19 months ago

In 54893:

Revert [54892] from the 6.1 branch.

This will be added again once committed to trunk first.

See #57163.

#19 @ocean90
19 months ago

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

In 54894:

Query: Account for primed post caches without primed post meta/term caches.

In [54352] update_post_caches() was replaced by _prime_post_caches() to reduce excessive object cache calls. That's because _prime_post_caches() checks first if post IDs aren't already cached. Unfortunately this becomes an issue if a post itself is cached but not the meta/terms.
To fix this regression, _prime_post_caches() now always calls update_postmeta_cache() and update_object_term_cache() depending on the arguments passed to it. Both functions internally check whether IDs are already cached so the fix from [54352] remains in place.

Props peterwilsoncc, spacedmonkey, ocean90.
Fixes #57163.

#20 @ocean90
19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 in reply to: ↑ 8 @ocean90
19 months ago

Replying to ocean90:

_prime_comment_caches(), _prime_term_caches(), and _prime_site_caches() are having the same issue. So whatever solution we'll use here should also get applied to them. Will create a follow-up ticket.

Done in #57227.

#23 @SergeyBiryukov
15 months ago

  • Milestone changed from 6.1.2 to 6.2.1

Moving to 6.2.1, as there are no plans for 6.1.2 at this time.

#24 @SergeyBiryukov
15 months ago

  • Milestone changed from 6.2.1 to 6.2
  • Resolution set to fixed
  • Status changed from reopened to closed

This is already included in the 6.2 branch as of [55504], no backport needed.

Note: See TracTickets for help on using tickets.