WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 8 months ago

#40669 closed defect (bug) (fixed)

Proper query cache invalidation on queries meta queries

Reported by: spacedmonkey Owned by: boonebgorges
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch dev-feedback needs-unit-tests
Focuses: performance Cc:

Description

Currently, terms and comments have query caching and meta query support. Queries are cached using the use the last_changed in the object cache group. Query caches are invalidated (busted) when the last_changed value is changed, when a term / comment is changed / created. However, when meta on these objects are changed / deleted, query are not invalidated. Example.

  • Run a comment query with a meta query foo=bar. Get 4 results.
  • add_comment_meta to a new comment.
  • Run a comment query with a meta query foo=bar. It will return cached result of 4, even through the result should be 5.

This is an issue, if plugin developers, use the meta functions to add meta to an object, queries should still invalidate.

Attachments (4)

40669.diff (5.5 KB) - added by spacedmonkey 14 months ago.
40669.2.diff (1.5 KB) - added by spacedmonkey 12 months ago.
40669-comments.diff (1.5 KB) - added by spacedmonkey 10 months ago.
40669-posts.diff (2.3 KB) - added by spacedmonkey 10 months ago.

Download all attachments as: .zip

Change History (17)

@spacedmonkey
14 months ago

#1 @spacedmonkey
14 months ago

  • Keywords has-patch dev-feedback needs-unit-tests added

In my first patch, I have changed the meta api to add a new a last_changed value in cache. I have then salted the query cache keys with meta cache, only if meta query is set. There is no need, if the query will not join to the meta table.

Related: #40229 #38025 #22176

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


12 months ago

#3 @boonebgorges
12 months ago

  • Component changed from Options, Meta APIs to Query
  • Milestone changed from Awaiting Review to 4.9

Thanks for the patch.

Worth noting that this is *not* currently a bug in term queries, because *_term_meta() busts the 'terms' incrementor. (You have removed this implementation and replaced it with your own strategy in 40669.diff.) At a glance, it does seem to be a bug in comment queries, though we should have tests that demonstrate this.

What's the advantage of having a separate 'comment_meta' and 'term_meta' incrementor? The main argument I can think of is: The cache for queries that don't have a 'meta_query' could potentially be longer-lasting if kept separate. However, I'd argue that if you aren't using 'meta_query' very often, then you probably aren't adding/updating/deleting meta very often either, so the issue of frequent invalidation is probably mostly moot. Generally, it doesn't seem worth the additional maintenance overhead to have a separate incrementor.

#4 @spacedmonkey
12 months ago

In 40669.2.diff I have a test that proves that add_comment_meta doesn't invalid caches correctly.

Admittedly term meta / term query doesn't have this issue as the last changed for term is changed whenever any term meta function is used.

This change is not only a bug fix but a performance change. Currently when you add any term meta, it invalidate all term queries. This is fine, but extreme. If a value of say comment meta is changed, this only effects queries with a meta query, as this joins to the meta table. Most queries in core and plugins do not have meta queries and are invalidated unnecessarily.

In #40229 and #38025 we have discussed adding meta queries to the network / site query classes. Both of these classes are used in the bootstrap process, so it is important that caching works well for these classes. It is likely the network options and blog meta might be updated a lot, it would effect performance if the queries were always being invalidated.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


12 months ago

#6 @boonebgorges
12 months ago

Currently when you add any term meta, it invalidate all term queries. This is fine, but extreme. If a value of say comment meta is changed, this only effects queries with a meta query, as this joins to the meta table. Most queries in core and plugins do not have meta queries and are invalidated unnecessarily.

Yes, but as I mentioned above, the real-life savings here are very modest. Updates to comment/term meta will often, if not usually, be accompanied by a comment/term update, which will result in the same invalidation.

Moreover: If we introduced comment_meta and term_meta incrementors, they would have to be invalidated every time the comment or term incrementors were busted; queries using 'meta_query' will be affected by general term/comment updates. And this is something that can't easily be done automatically in update_meta() etc, because of the way we have named our cache groups. This is part of the maintenance overhead I talked about earlier: we have to remember to bump multiple incrementors to avoid corner-case invalidation bugs. The benefit for this is very modest, so I think it's not worth it.

So I'd advocate instead for taking the strategy from *_term_meta() and applying it to comment meta as well.

#7 @boonebgorges
12 months ago

(thanks for the test, btw :) )

#8 @spacedmonkey
10 months ago

@boonebgorges I have added patches for posts and comments to do that same cache group invalidation that term meta does. My patches for network (#37181) and site (#37923) meta already do this.

Worth noting that users doesn't to last_changed so it is not required for this type.

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


8 months ago

#10 @melchoyce
8 months ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#11 @boonebgorges
8 months ago

In 41848:

Invalidate comment query cache when modifying comment meta.

Comment queries are sensitive to comment meta due to the meta_query
parameter, so the cache must be invalidated when comment meta is changed,
added, or deleted.

Props spacedmonkey.
See #40669.

#12 @boonebgorges
8 months ago

@spacedmonkey - Thanks again for the updated patches, and sorry for the delay in review.

The comment query issue is fixed in [41848]. I modified your tests a bit so that they were in a more appropriate file, so that they generated fewer fixtures, and so that they tested results a bit more precisely.

Post queries are currently uncached except for in the case of get_pages(). So the bug fixed by 40669-posts.diff is in get_pages() specifically. I'll add tests that reflect this. If we one day use cache incrementors to cache other post queries, we can think about making the tests more general.

#13 @boonebgorges
8 months ago

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

In 41849:

Bump 'posts' query cache incrementor when modifying postmeta.

This ensures that the get_pages() query cache doesn't go stale when
postmeta is modified.

Props spacedmonkey.
Fixes #40669.

Note: See TracTickets for help on using tickets.