Proper query cache invalidation on queries meta queries

[spacedmonkey]

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.

[spacedmonkey]

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

[boonebgorges]

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.

[spacedmonkey]

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.

[boonebgorges]

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.

[boonebgorges]

(thanks for the test, btw :) )

[spacedmonkey]

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

[boonebgorges]

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.

[boonebgorges]

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

[boonebgorges]

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.