Make WordPress Core

Opened 12 years ago

Closed 21 months ago

Last modified 16 months ago

#22176 closed enhancement (fixed)

Cache the results of the posts_request_ids query

Reported by: ryan's profile ryan Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.4.2
Component: Query Keywords: has-patch has-unit-tests has-dev-note
Focuses: performance Cc:

Description (last modified by 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

Attachments (9)

22176.diff (1.3 KB) - added by ryan 12 years ago.
The gist
22176.2.diff (2.7 KB) - added by ryan 12 years ago.
found_posts caching
22176.3.diff (4.7 KB) - added by ryan 12 years ago.
Add context arg to clean_post_cache(). Don't update last_changed for certain contexts.
22176.patch (7.8 KB) - added by spacedmonkey 7 years ago.
22176.2.patch (7.8 KB) - added by spacedmonkey 7 years ago.
22176.3.patch (8.0 KB) - added by spacedmonkey 7 years ago.
22176.4.diff (20.9 KB) - added by spacedmonkey 7 years ago.
Screenshot 2022-07-22 at 18.48.11.png (11.3 KB) - added by spacedmonkey 2 years ago.
Before patch
Screenshot 2022-07-22 at 18.47.31.png (11.8 KB) - added by spacedmonkey 2 years ago.
After patch

Download all attachments as: .zip

Change History (80)

#1 @scribu
12 years ago

  • Cc scribu added

@ryan
12 years ago

The gist

#2 @ryan
12 years ago

  • Description modified (diff)

@ryan
12 years ago

found_posts caching

#3 @batmoo
12 years ago

  • Cc batmoo@… added

#4 @johnbillion
12 years ago

  • Cc johnbillion added

#5 @ryan
12 years ago

Related: #15565

@ryan
12 years ago

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

#6 @nacin
12 years ago

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.

#7 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

#8 @chriscct7
9 years ago

  • Keywords has-patch needs-refresh added

#9 @ocean90
8 years ago

#36792 was marked as a duplicate.

#10 @ocean90
8 years ago

  • Component changed from Cache API to Query
  • Focuses performance added

This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.


8 years ago

@spacedmonkey
7 years ago

#12 @spacedmonkey
7 years ago

  • Keywords needs-unit-tests 2nd-opinion added; needs-refresh removed

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.

#13 @spacedmonkey
7 years ago

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.

#14 @DrewAPicture
7 years ago

@spacedmonkey Looks like we still need some tests here.

@spacedmonkey
7 years ago

#15 @spacedmonkey
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

#16 @spacedmonkey
7 years ago

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.

#17 @spacedmonkey
2 years ago

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

#18 @spacedmonkey
2 years ago

  • Milestone changed from Future Release to 6.0

#19 @spacedmonkey
2 years ago

  • Milestone changed from 6.0 to Future Release

#20 @peterwilsoncc
2 years ago

  • Keywords early added

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.

#22 @spacedmonkey
2 years ago

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

#23 @mukesh27
2 years ago

  • Keywords needs-testing added

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.

#24 @mukesh27
2 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 6.1

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.

#25 @costdev
2 years ago

  • Keywords changes-requested added

#26 @spacedmonkey
2 years ago

  • Keywords needs-refresh changes-requested removed

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

spacedmonkey commented on PR #2684:


2 years ago
#27

CC @tillkruss

#28 follow-up: @peterwilsoncc
2 years ago

  • Keywords needs-dev-note added

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.

Last edited 2 years ago by peterwilsoncc (previous) (diff)

SergeyBiryukov commented on PR #2684:


2 years ago
#29

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 :)

spacedmonkey commented on PR #2684:


2 years ago
#30

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 ?

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


2 years ago

#32 in reply to: ↑ 28 ; follow-up: @dd32
2 years ago

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.

#33 @spacedmonkey
2 years ago

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.

#34 @peterwilsoncc
2 years ago

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

#35 in reply to: ↑ 32 @SergeyBiryukov
2 years ago

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 :)

This ticket was mentioned in PR #3011 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#36

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

This ticket was mentioned in PR #3011 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#37

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

#38 @spacedmonkey
2 years ago

Thanks for the feedback @peterwilsoncc @dd32 @SergeyBiryukov

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

#39 @peterwilsoncc
2 years ago

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.

#40 @spacedmonkey
23 months ago

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.

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


23 months ago

peterwilsoncc commented on PR #3011:


23 months ago
#42

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.

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


22 months ago

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


22 months ago

#45 @peterwilsoncc
22 months ago

  • Keywords commit added
  • Owner changed from spacedmonkey to peterwilsoncc

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

peterwilsoncc commented on PR #2684:


22 months ago
#46

Closing this off as superseded by #3011 .

spacedmonkey commented on PR #3011:


22 months ago
#47

🎉

#48 @peterwilsoncc
22 months ago

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

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.

peterwilsoncc commented on PR #3011:


22 months ago
#49

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

#50 @Chouby
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#51 @johnbillion
22 months ago

  • Keywords has-patch 2nd-opinion has-unit-tests early needs-testing commit removed

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


22 months ago
#52

  • Keywords has-patch has-unit-tests added

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.

#53 @peterwilsoncc
22 months ago

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

#54 @Chouby
22 months ago

@peterwilsoncc Your PR works for me.

#55 @peterwilsoncc
22 months ago

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.

peterwilsoncc commented on PR #3137:


22 months ago
#56

@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

#57 @peterwilsoncc
22 months ago

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.

#59 @spacedmonkey
22 months ago

  • Keywords add-to-field-guide added

#60 @desrosj
21 months ago

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

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

#61 @spacedmonkey
21 months ago

Made a start of dev notes here.

#63 @spacedmonkey
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

This ticket was mentioned in PR #3353 on WordPress/wordpress-develop by mukeshpanchal27.


21 months ago
#66

Test PR.

#67 @spacedmonkey
21 months ago

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.

#68 @spacedmonkey
21 months ago

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

spacedmonkey commented on PR #3348:


21 months ago
#69

Committed

#70 @milana_cap
21 months ago

  • Keywords has-dev-note added; needs-dev-note add-to-field-guide removed

This ticket was mentioned in Slack in #docs by philipp.bammes. View the logs.


16 months ago

Note: See TracTickets for help on using tickets.