Cache the results of the posts_request_ids query

[ryan]

We are to the point where we could replace the advanced post cache plugin with something in core that is far simpler. We're most of the way there since introducing the split query. And with #22024 we have a good way of doing per-blog cache invalidation for classes of objects, which would be needed by this. Leveraging wp_cache_get_multi() as suggested in #22174 would provide a complete replacement for the adv post cache plugin. ​http://plugins.svn.wordpress.org/advanced-caching/trunk/advanced-caching.php

[ryan]

The gist

[ryan]

found_posts caching

[ryan]

Related: #15565

[ryan]

Add context arg to clean_post_cache(). Don't update last_changed for certain contexts.

[nacin]

A parent deletion could hypothetically change a query that is based on post_parent, or eventual post_parent_in / not_in, child_of, exclude_tree query variables.

[ocean90]

#36792 was marked as a duplicate.

[spacedmonkey]

I have spent sometime on this ticket as I think it is much simpler that originally thought.

• The cache key generated has the post types array. The idea behind this is when new post types are registered caches are cleared.
• The fields element is removed, so that all formats are wp query are stored in the same cache as a list of ids.
• The return points from id and id=>parent formats have been changed. This makes code a little cleaner. It also means that all the filters are fired more, making this code much more hackable.
• I have reused the cache_filter param and changed what it is used for. I believe this is a nice solution.
• Added a param update_post_cache, bring query class into line with other newer query classes like wp_site_query.

I am worried about how filters effect this code. This code has tonnes of filters, any of which may break the caching. Currently none of the core tests are failing.

[spacedmonkey]

After talking to @boonebgorges about this ticket, I have updated it.
Key changes.

• Now it obeys the posts_pre_query filter
• If tax_query is set, salt the cache key with term last changed.
• Cache key is now generated from the sql string. This is because of the filters on the sql string.

I think will need to look into #40669 to make sure post meta query run correctly.

[DrewAPicture]

@spacedmonkey Looks like we still need some tests here.

[spacedmonkey]

22176.4.diff Adds unit tests.

Other changes include

• Cache generation is outside of if statement as cache key is used in other parts of code.
• The posts cache group is invalidated when meta data is added/deleted/updated. See #40669
• Cache key is salted with last changed of group terms. This helps invalidation of taxonomy queries.

[spacedmonkey]

My last patch is going a little bit out of scope. It become about refactoring WP_Query to make the code cleaner. In #41678 does much of the refactoring in this ticket and makes different return types run through filters. As more the filters run on more return types, this caching can happen outside of core.

[peterwilsoncc]

This sprung to mind as there are a few add caching to some post function tickets that would be covered by this.

I've put the early keyword on it as it involves both caching and Query.

Trac ticket: https://core.trac.wordpress.org/ticket/22176

[spacedmonkey]

I have refreshed the patch here, in hope of getting this into WP 6.1.

[mukesh27]

Can you please add a new $post_query_cache doc block with the upcoming version number, like 6.1?

* @since 6.1.0 Introduced the `$post_query_cache` parameter.

needs-testing keyword added for testing.

[mukesh27]

The PR needs to be refreshed against the latest changes, @spacedmonkey can you please update the PR and also add a unit test in the PR?

In my quick check of unit tests in 22176.4.diff​ i found that $p2 = self::factory()->post->create(); is not needed, so can you please remove it?

For visibility and testing purposes, I'm going to set the Milestone for the next major release 6.1.

[spacedmonkey]

As requested unit tests have been added and are all passing. I think this patch is getting close.

CC @tillkruss

[peterwilsoncc]

At a technical level, I agree with @spacedmonkey that this patch is getting very close.

A final decision to be made is to decide upon the default setting for caching, post_query_cache:

• always on: true
• always off: false -- with core functions updated to default on
• conditionally true

The sticking point of always true is that some plugins and/or custom WP-CLI commands may be expecting an uncached value. The simplest example of this is a custom importer that uses WPDB rather than wp_insert_post().

Discussing this issue with Jonny, our current thinking is that conditionally defaulting to enable the cache is probably the best option. Certainly as an initial step.

A dev note would be required to advise developers to use this flag.

Thinking of the naming, based on the parameter description, I thought $cache_results would be a more appropriate name, but WP_Query already has exactly that parameter as of WordPress 3.0, see r14310 / #12989. At a glance, it's not quite clear what is the difference between the two. It seems like $cache_results has a limited effect at the moment, but $post_query_cache does exactly what $cache_results should do.

It appears that the initial patches on #22176 expanded the usage of $cache_results, instead of introducing a new parameter, but I can't see any further discussion on why that approach was abandoned. Should it be explored further? Otherwise, the two parameters can get pretty confusing :)

Thinking of the naming, based on the parameter description, I thought $cache_results would be a more appropriate name, but WP_Query already has exactly that parameter as of WordPress 3.0, see r14310 / #12989. At a glance, it's not quite clear what is the difference between the two. It seems like $cache_results has a limited effect at the moment, but $post_query_cache does exactly what $cache_results should do.

It appears that the initial patches on #22176 expanded the usage of $cache_results, instead of introducing a new parameter, but I can't see any further discussion on why that approach was abandoned. Should it be explored further? Otherwise, the two parameters can get pretty confusing :)

I didn't mention it in the ticket, but I decided to side step this discussion by just adding a new param. But I am not sure I am fully happy with changing the behaviour of existing param. Looking at the plugin repo there are ​lots of uses of the cache_results param. The behaviour of the param needs to stay the same backwards compat. Another downside of using this param, is that is defaults to true, which is not the expected behaviour in this new param.

The cleanest way IMO, is add a new param. Unless you have other ideas @SergeyBiryukov ?

[dd32]

Replying to peterwilsoncc:

A final decision to be made is to decide upon the default setting for caching, post_query_cache:
...
The sticking point of always true is that some plugins and/or custom WP-CLI commands may be expecting an uncached value.

The question arrises here - Does the benefit of providing caching outweigh the slight potential back-compat breakage? I think it does, and that anything expecting uncached data should be specifying the cache_results flag to bypass it already.

In my mind - altering the database without going through WordPress APIs (which would change the cache key) is something that already results in unexpected results, for example, changing a post_status in the DB you then need to call (at a minimum) clean_post_cache( $post_id ) (Which updates/cleans the post cache, and bumps the posts last_changed key). That means that while there's a chance of BC breakage here, it's far more likely that affected code is already broken - although maybe not obviously.

If ultimately the risk-appetite isn't there from the committer(s), I would like to suggest an alternative way forward: Conditionally-enabled in X.Y + Dev Note, Enabled-by-default in Nightly builds and in X.Y+1 or X.Y+2 release builds. The reverse of deprecation effectively.

[spacedmonkey]

Thanks for your feedback @dd32

So my plan is the following.

Release 1.
Introduce new param, post_query_cache for caching, default to false.
Opt in the following.

• Main query on page.
• WP_Query in recent posts block / widget.
• get_user_data_from_wp_global_styles
• block_core_navigation_get_first_non_empty_navigation
• render_block_core_template_part
• get_block_templates

Dev note to explain change.

Release 2.
Opt in all other usage of WP_Query in core, baring get_posts.

Release 3.
Make post_query_cache default to true.

This is all based on the idea, that as cache_results as basically not ever cached anything, that developers do not know about it / use it. If the patch does use cache_results param and we trust this params value, then the patch is much easier and we can opt users into this function with no issue.

Caching invalidating is pretty solid as the moment. In my patch the biggest pain points where.

• Sticky posts.
• Taxonomy queries.
• Querying by parent, such as media, revision and auto saves.

It is worth noting that I am willing to risk enabling caching by default. But I am trying to de-risk this. If there is an support for enabling by default for other committers, I will update the patch.

[peterwilsoncc]

I could be convinced either way but am leaning to on by default from the initial commit:

• reduces the complexity of the change
• avoids a new argument that will eventually have the same default as the existing, similarly named one
• Dion makes a good point about not using existing WP APIs requiring an acceptance of risk (which can be mitigated by flushing the last_changed cache)
• the change needs a dev-note either way

[SergeyBiryukov]

Replying to dd32:

The question arrises here - Does the benefit of providing caching outweigh the slight potential back-compat breakage? I think it does, and that anything expecting uncached data should be specifying the cache_results flag to bypass it already.

Yes, similar thoughts here.

Replying to peterwilsoncc:

I could be convinced either way but am leaning to on by default from the initial commit

I'm leaning to on too, not a fan of a new parameter that ends up being pretty much the same as the existing one, or has a similar purpose where the difference is not quite obvious. Would rather extend the existing one and see what may break in practice, if anything :)

Another version of ​https://github.com/WordPress/wordpress-develop/pull/2684 that uses the cache_results param.

Trac ticket: https://core.trac.wordpress.org/ticket/22176

Another version of ​https://github.com/WordPress/wordpress-develop/pull/2684 that uses the cache_results param.

Trac ticket: https://core.trac.wordpress.org/ticket/22176

[spacedmonkey]

Thanks for the feedback @peterwilsoncc @dd32 @SergeyBiryukov

I have created a new PR use uses the cache_results param. See ​#3011.

[spacedmonkey]

Before patch

[spacedmonkey]

After patch

[peterwilsoncc]

I've put some notes on the version two PR (always on caching).

I'll start pushing to Jonny's PR (or create a fork) while he is AFK for the next few weeks.

[spacedmonkey]

Couple of other things to consider.

There are plugins like ​advanced-post-cache and ​Enhanced Post Cache that already add caching to the WP_Query class. Both of which use the posts_request_ids filter to bypass the normal query. We maybe want to depercate this filter in the patch and recommend the use of posts_pre_query filter, which will play nicely with this change. Worse case, is that posts are double cached, by there maybe some issues there with cache invalidation.

We maybe also want to consider plugins like elasticpress, that bypass normal queries using posts_pre_query and ensure that there are never cached.

I ended up removing the default filters on sticking and unsticking a post as I could not find a way in which they were needed.

A call to WP_Query that accounts for sticky posts

• makes a DB query ignoring sticky posts (cached)
• makes a WP_Query for posts in the sticky post option
• munges the arrays as needed on output (post caching)

A call to WP_Query that doesn't account for sticky posts makes the first query only.

When sticky posts change, the sticky WP_Queries differ as the post IDs in the option change.

[peterwilsoncc]

Dion has given the PR an approval, self assigning for commit.

Closing this off as superseded by #3011 .

🎉

[peterwilsoncc]

In 53941:

Query: Cache post ID database query within WP_Query.

Add object caching to the first database query in WP_Query, ie the database query for post IDs that match the desired result. Randomly ordered queries remain uncached as they are not intended to return consistent results.

Caching of ID queries is enabled by default, per the post object, term and meta caches.

Props spacedmonkey, aaroncampbell, batmoo, chriscct7, costdev, dd32, drewapicture, johnbillion, mukesh27, nacin, ocean90, peterwilsoncc, ryan, scribu, sergeybiryukov, tillkruss.
Fixes #22176.

Committed in https://core.trac.wordpress.org/changeset/53941 / 7f7d616d822b79c952cbd6a3046a6f6a8aa5a35e

[Chouby]

I have written a phpunit test which seems to break due to [53941]:

<?php
public function test_53941() {
        $term_id = $this->factory->term->create( array( 'taxonomy' => 'category' ) );
        $post_id = $this->factory->post->create();

        $args = array(
                'fields'    => 'ids',
                'tax_query' => array(
                        array(
                                'taxonomy' => 'category',
                                'terms'    => array( $term_id ),
                                'operator' => 'NOT IN',
                        ),
                ),
        );

        $post_ids = get_posts( $args );
        $this->assertNotEmpty( $post_ids );

        wp_set_object_terms( $post_id, array( $term_id ), 'category' );

        $post_ids = get_posts( $args );
        $this->assertEmpty( $post_ids );
}

This test passes in WP 6.0.1 as expected. It fails on the second assertion in the current development version.

https://core.trac.wordpress.org/ticket/22176

cc @spacedmonkey, @tillkruss -- still a work in progress in case something else comes in but making sure you have eyes on it.

[peterwilsoncc]

@Chouby thanks for your report and continued testing of pre-release software.

I was able to reproduce the bug and have a fix in the ​linked pull request containing follow up work.

I've added a modified version of your test to the caching test suite. I needed to modify the assertions to contains/not contains as the shared fixtures mean some posts are expected.

[Chouby]

@peterwilsoncc Your PR works for me.

[peterwilsoncc]

Thanks for testing the PR, @Chouby.

I'll commit the fix to the taxonomy based queries but leave the ticket open for the time being in case any other issues come in during the release cycle.

@costdev

• In 06f2e1bd74c55da6a6afa140bfec39fa35cd33d2 I've added @covers WP_Query::get_posts at the class level
• In fc62eae67c5f98fdcf490cc6c3a9b4413ed477a4 I've renamed the methods
• In d0a7e8bea8eb5f21aa6340037f668ca19a6b39c1 I've modified the variable names to indicate which query the posts belong to

[peterwilsoncc]

In 54111:

Query: Improve WP_Query's cache key generation for taxonomy queries.

Modify how WP_Query determines whether a database query contains a taxonomy component and accounts for term changes when generating the cache key. This presents a stale cache been used under some circumstances.

Props Chouby, costdev, peterwilsoncc.
See #22176.

Merged in https://core.trac.wordpress.org/changeset/54111 / 48f17267fb

[desrosj]

I believe that everything has been addressed for this one. Re-closing as fixed. If that's incorrect, please feel free to reopen.

[spacedmonkey]

Made a start of dev notes ​here.

Trac ticket: https://core.trac.wordpress.org/ticket/22176

[spacedmonkey]

Reopening, as I have discovered a couple of issues when persistent Object Caching.

• Issue 1 - Cache add is called a lot when persistent Object Caching is enabled. This is because update_post_caches is called. This function should not be called when object cache is enabled.
• Issue 2 - Cache set is called every WP_Query run, not only the first.

CC @peterwilsoncc to review my ​PR.

Memcached failures look to be true positives ​https://github.com/WordPress/wordpress-develop/actions/runs/3139034236/jobs/5099033209#step:19:313

Left one nitpick suggestion ​https://github.com/WordPress/wordpress-develop/pull/3348#discussion_r981996311 if that make sense.

Test PR.

[spacedmonkey]

In 54352:

Query: Save excessive cache add and sets in WP_Query.

In [53941] database query caching was added to WP_Query. However on sites with persistent object caching enabled, this resulted in a high number of unnecessary cache set and adds being run on every request. Caches are not set, if the query cache already exists and is cached. Replace usage of update_post_caches with _prime_post_caches to ensure that only posts that are not in cache are primed.

Props spacedmonkey, peterwilsoncc, mukesh27.
See #22176.

Committed

[milana_cap]

Dev note: ​https://make.wordpress.org/core/2022/10/07/improvements-to-wp_query-performance-in-6-1/