Make WordPress Core

Opened 19 months ago

Closed 9 months ago

Last modified 9 months ago

#57296 closed enhancement (fixed)

Force split queries in WP_Query in object caching is enabled.

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-unit-tests needs-testing needs-dev-note add-to-field-guide needs-patch revert 2nd-opinion
Focuses: performance Cc:

Description

In WP_Query, there is a abililty to split the query to just run database query by IDs. If using object caching, this should always be enabled by default.

Attachments (2)

Screenshot 2023-09-05 at 12.25.30.png (92.8 KB) - added by spacedmonkey 10 months ago.
Trunk - query time
Screenshot 2023-09-05 at 12.25.15.png (100.1 KB) - added by spacedmonkey 10 months ago.
PR - query time

Download all attachments as: .zip

Change History (22)

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


19 months ago
#1

  • Keywords has-patch added

#2 follow-up: @peterwilsoncc
19 months ago

Looking at #20628, it appears that when queries were initially split it was always on but caused problems in some circumstances.

There's a little detail on the ticket but I suspect some discussion happened off-ticket too.

I've been considering something similar to this ticket but defaulting to always splitting queries, regardless of the use of a persistent cache or not.

@SergeyBiryukov, do you have any additional background for #20628? It was some time ago so I realise you may not recall.

#3 in reply to: ↑ 2 @SergeyBiryukov
19 months ago

Replying to peterwilsoncc:

do you have any additional background for #20628?

#20621 appears to provide some more context:

Since #15459 was later fixed in 4.2, this seems worth revisiting.

#4 @spacedmonkey
17 months ago

  • Keywords has-unit-tests added

Unit tests added.

@peterwilsoncc commented on PR #3733:


17 months ago
#5

The unit test failures in test_wp_get_nav_menu_items_cache_primes_posts on memcached configs look legitimate. I guess because the query used in those tests are now split.

#6 @spacedmonkey
12 months ago

  • Milestone changed from Future Release to 6.4

@spacedmonkey commented on PR #3733:


11 months ago
#7

@peterwilsoncc Unit tests are fixed. The unit test needed to updated, as there is a change in behaviour.

#8 @oglekler
10 months ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


10 months ago

#10 @joemcgill
10 months ago

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

@spacedmonkey
10 months ago

Trunk - query time

#11 @spacedmonkey
10 months ago

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

In 56513:

Query: Use split queries in WP_Query if persistent object caching is enabled.

Prior to this commit, the WP_Query class split the query for posts into two database queries. First, it initiated an ID-based query to retrieve post IDs, followed by a call to _prime_post_caches to fetch post objects if they weren't already in memory. This splitting of queries was limited to cases where fewer than 500 posts were being requested, to prevent a potentially large database query within the IN statement in _prime_post_caches.

However, this limitation becomes unnecessary when a persistent object cache is enabled, as the post objects can be efficiently retrieved from the fast object cache. This commit transfers the responsibility of fetching posts to the object cache, which not only speeds up the process but also reduces the strain on the database server.

Props peterwilsoncc, spacedmonkey, SergeyBiryukov.
Fixes #57296.

#13 @tillkruess
9 months ago

  • Keywords needs-dev-note add-to-field-guide added

@spacedmonkey: I've received support requests from 5 plugin authors complaining that split_the_query breaks their plugin. This change must be documented in detail in the field guide.

Either way this will cause wide-spread issues with persistent object caching plugins and it would be nice to have a dedicated post about this we can link to, so it doesn't get lost in the field guide.

For example: https://github.com/wpmetabox/mb-relationships/commit/06aa11e0d99be5663622c06eb39b0de9b0817bd9

#14 follow-up: @spacedmonkey
9 months ago

@tillkruess Do you mind creating another ticket for this issue?

I am still not sure I understand the issue related to this commit. split_the_query filter has existed since WP 3.4. This change only forces split queries if the site is running object caching. But most queries were already split, as the have a limit on them. This only changes if query is run on a site with object cache and not limit is set.

I agree this needs a dev note and some developer education.

#15 in reply to: ↑ 14 @azaozz
9 months ago

  • Keywords needs-patch revert 2nd-opinion added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to spacedmonkey:

This only changes if query is run on a site with object cache and not limit is set.

If plugins are broken seems this is a "breaking change" or perhaps there is more to it than that?

I agree this needs a dev note and some developer education.

Dev. notes and developer education are not substitute for backwards compatibility! A regression is a regression no matter how much you "educate" about it. Regressions are the worst type of bugs that can be introduced and fixing them is not optional.

If the regression cannot be fixed imho this change should be reverted.

Last edited 9 months ago by azaozz (previous) (diff)

#16 @spacedmonkey
9 months ago

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

After a chat with @tillkruess, the issue he highlights are not related to this change and will be handled in another ticket. The issue is using the split_the_query filter, is not related to this change as it was added in WP 3.1.

#17 @tillkruess
9 months ago

Correct, thanks for talking this through with me, @spacedmonkey.

#18 @spacedmonkey
9 months ago

Created a follow on it from #59514.

#19 @webcommsat
9 months ago

@spacedmonkey does this mean that this is no longer required for 'needs-dev-note' and 'add-to-field-guide'? Thanks

#20 @spacedmonkey
9 months ago

@webcommsat No. This still needs a dev note.

I am currently working on it here.

Note: See TracTickets for help on using tickets.