Make WordPress Core

Opened 2 years ago

Closed 17 months ago

Last modified 16 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 17 months ago.
Trunk - query time
Screenshot 2023-09-05 at 12.25.15.png (100.1 KB) - added by spacedmonkey 17 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.


2 years ago
#1

  • Keywords has-patch added

#2 follow-up: @peterwilsoncc
2 years 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
2 years 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
2 years ago

  • Keywords has-unit-tests added

Unit tests added.

@peterwilsoncc commented on PR #3733:


2 years 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
20 months ago

  • Milestone changed from Future Release to 6.4

@spacedmonkey commented on PR #3733:


19 months ago
#7

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

#8 @oglekler
18 months ago

  • Keywords needs-testing added

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


18 months ago

#10 @joemcgill
18 months ago

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

@spacedmonkey
17 months ago

Trunk - query time

#11 @spacedmonkey
17 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
17 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
17 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
17 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 17 months ago by azaozz (previous) (diff)

#16 @spacedmonkey
17 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
17 months ago

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

#18 @spacedmonkey
16 months ago

Created a follow on it from #59514.

#19 @webcommsat
16 months ago

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

#20 @spacedmonkey
16 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.