Make WordPress Core

Opened 8 months ago

Closed 11 days ago

Last modified 11 days ago

#59442 closed defect (bug) (fixed)

Duplicate query in WP_Query

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.2
Component: Query Keywords: has-patch early has-unit-tests commit
Focuses: performance Cc:

Description

When a site using a classic theme and has sticky posts, that can result in duplicate query. ( See attached screenshot ). This is because post_type variable passed the sticky sub query, is empty string on the first run. See the line. This results in different cache query key being generating, resulting in duplicate queries.

Steps to replicate.

  1. Set theme to 2016.
  2. Import theme data test data.
  3. Go to home page.
  4. Open query monitor, see duplicate query.

Attachments (3)

Screenshot 2023-09-25 at 12.26.09.png (362.9 KB) - added by spacedmonkey 8 months ago.
Screenshot 2023-09-25 at 12.33.38.png (499.7 KB) - added by spacedmonkey 8 months ago.
59442.diff (965 bytes) - added by peterwilsoncc 4 months ago.

Download all attachments as: .zip

Change History (50)

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


8 months ago
#1

  • Keywords has-patch added

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


8 months ago

#4 @spacedmonkey
8 months ago

  • Milestone changed from Awaiting Review to 6.4

#5 @hellofromTonya
8 months ago

  • Keywords dev-feedback added

As this is touching WP_Query, I'm thinking it needs to be an early candidate. Why? Touching it is risky. Committing early in the alpha cycle gives a long soak time for follow-ups, testing, and feedback. Touching it during the 6.4 Beta cycle makes me nervous.

@spacedmonkey is this fix critical for 6.4? What do you think of moving it to early 6.5?

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


8 months ago

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


8 months ago

#8 @thekt12
8 months ago

One of the main reason we get multiple queries for the same SQL is the hash takes serialised args as string for hashing, leading to a different key for even slightest of change in args.

I have documented it in ->
https://core.trac.wordpress.org/ticket/59492

Following PR, solves the current issue along with possibility of any more duplicates in different scenerio.
https://github.com/WordPress/wordpress-develop/pull/5347/files

#9 @swissspidy
8 months ago

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

#10 @thekt12
8 months ago

Sticking to the original problem, this PR https://github.com/WordPress/wordpress-develop/pull/5295 fixes the issue.

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


8 months ago

#12 @spacedmonkey
8 months ago

  • Keywords commit added

PR looks good to me. Marking as ready to commit.

#13 @spacedmonkey
8 months ago

  • Keywords early added
  • Milestone changed from 6.4 to 6.5
  • Owner changed from spacedmonkey to joemcgill

Based on feedback from @hellofromTonya and @joemcgill I am going to punt this to WP 6.5. Marking as early, in hope of commit early and giving the whole release cycle to test. Assigning to Joe to own.

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


7 months ago

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


4 months ago

#16 @joemcgill
4 months ago

  • Keywords changes-requested added

@spacedmonkey and @thekt12, I left some feedback on the PR that I'd like to see addressed before I would feel comfortable with this change in it's current state. I acknowledge that we're getting to this a bit later in the 6.5 release cycle than Jonny had hoped when he marked it for early commit, so this may need to be punted if we can't resolve quickly.

#17 @spacedmonkey
4 months ago

@joemcgill Answered feedback.

@peterwilsoncc
4 months ago

#18 @peterwilsoncc
4 months ago

I've taken a quick look at the linked pull request and am having trouble understanding its complexity.

It lacks test and it's possible it's missing something, or I am misunderstanding something, but 59442.diff seems like a simpler approach.

#19 @spacedmonkey
4 months ago

@peterwilsoncc The duplicate query comes from this lines. In these contexts, $post_type is empty, when it should be a valid post type string. Changing the $post_type variable also effects this line. Passing post type in update_post_caches when before it wasn't might be a problem in some edge cases, like if you filter $this->posts to have mixed post types.

#20 @peterwilsoncc
4 months ago

As the $post_type is defined only where the WHERE clause is set to a single post type, I don't think it's a problem for this ticket which should allow some variation of the simpler patch to be used.

update_post_caches() is only called if $this->posts matches the unfiltered value. If the posts have been modified then _prime_post_caches() is used which allows for any post type to be primed.

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


4 months ago

#22 follow-up: @spacedmonkey
3 months ago

@peterwilsoncc @joemcgill If we are scale this patch back. I think we should consider the first version from @thekt12 of this patch. See thie commit.

I think we are a little overly worried about backwards compat here. It is an extreme edge case to break this code. If anything, I feel like the post type variable being set is a good thing.

I feel like this is really close to committed.

#23 in reply to: ↑ 22 @joemcgill
3 months ago

Replying to spacedmonkey:

@peterwilsoncc @joemcgill If we are scale this patch back. I think we should consider the first version from @thekt12 of this patch. See thie commit.

At first glance, this looks clearer than the final PR, but would appreciate @peterwilsoncc's thoughts. I would still like to see a unit test that demonstrates the issue in addition to the included test that improves coverage for the current behavior.

I think we are a little overly worried about backwards compat here. It is an extreme edge case to break this code. If anything, I feel like the post type variable being set is a good thing.

I feel like this is really close to committed.

I think it's always good to be extra cautious about backwards compatibility when modifying WP_Query. From what I can tell, this addresses an issue that is not that common (e.g., you have to have a sticky post active) and even then provides just a moderate performance benefit without any change to the end user experience, so I'd rather get good test coverage and consensus on the PR before committing.

#24 @spacedmonkey
3 months ago

@joemcgill I would like to see this committed in WP 6.5. I have approved the existing PR. I know it is overly careful with this variable but I personally think that is a good thing. Can we commit this before the beta 1 deadline and handle issues in follow up commits?

#25 @huzaifaalmesbah
3 months ago

I've tested 59442.diff And it works as expected and code is easy to understand.

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


3 months ago

#27 @joemcgill
3 months ago

  • Keywords commit removed
  • Milestone changed from 6.5 to 6.6
  • Owner joemcgill deleted

Replying to spacedmonkey:

I would like to see this committed in WP 6.5.

Sorry, I didn't see your response here prior to the beta release. Even so, I'm moving this to the 6.6 milestone and leaving unassigned for anyone else to pick up, but am happy to review again once updated.

The current PR from @thekt12 has not yet been updated to address the feedback from @peterwilsoncc and I.

Specifically, I think the following changes are needed:

  • Simplifying the approach to match either 59442.diff or the early commit referenced by @spacedmonkey above.
  • Adding a unit test that demonstrates the issue being fixed by this change (e.g., it should fail when run in isolation prior to the changes being applied)

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


2 months ago

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


7 weeks ago

#30 @joemcgill
7 weeks ago

  • Owner set to pbearne

@pbearne agreed to pick this up in today's #core-performance meeting. Reassigning so we can try to get this in early during the 6.6 cycle.

#31 @pbearne
7 weeks ago

I have not been able to create test that fails before the new code
Any pointers?

#32 @pbearne
7 weeks ago

The test wasn't loading the sticky post as get_post() is hardcode to ingore them

I am not sure this patch does anything

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


6 weeks ago

This ticket was mentioned in PR #6393 on WordPress/wordpress-develop by jonnynews.


5 weeks ago
#34

Post type handling in the WP_Query class has been improved. Now, if a post type is not defined, a default one will be set based on whether the post is an attachment, a page, or a regular post. This adjustment streamlines the code, improving both efficiency and readability.

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

#35 @spacedmonkey
5 weeks ago

@pbearne I have created a new patch. https://github.com/WordPress/wordpress-develop/pull/6393.

I am going to make some unit tests but I think I finally cracked it. Basically the issue, is that orderby and post types can be set to empty values in the args, so it results in duplicate queries.

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


5 weeks ago

drhsrushb commented on PR #6393:


4 weeks ago
#37

o address this, you may need to ensure that the post_type variable is properly set in the sticky subquery, especially on the initial run. Double-checking how the post_type variable is initialized and passed in the query could help resolve the issue. Additionally, reviewing caching mechanisms and how they handle queries with different keys might provide further insights into how to prevent duplicate queries in this scenario.

If you're comfortable sharing more details or code snippets, I'd be happy to provide more specific suggestions or assistance!

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


4 weeks ago

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


4 weeks ago

@peterwilsoncc commented on PR #6393:


3 weeks ago
#41

@spacedmonkey Sorry mate, I was thinking this block here. But it was good to catch the other spot.

https://github.com/WordPress/wordpress-develop/blob/55026bcad3380ec1ee30e95379ba47be3c84994a/src/wp-includes/class-wp-query.php#L2539-L2547

jonnynews commented on PR #6393:


3 weeks ago
#42

@spacedmonkey Sorry mate, I was thinking this block here. But it was good to catch the other spot.

https://github.com/WordPress/wordpress-develop/blob/55026bcad3380ec1ee30e95379ba47be3c84994a/src/wp-includes/class-wp-query.php#L2539-L2547

Good catch @peterwilsoncc. I have actioned your feedback found some more cases where keys would not match.

  • If post status is sent in a different order.
  • If post status is string and array.

I also improved the tests to use second query object and use the second query objects request. This highlighted a weird bug, where spaces in the query resulted in none match sql, meaning the keys do not match.

Tests are now passing.

#45 @spacedmonkey
2 weeks ago

Thanks for your feedback @rodionov201

The fix in the PR #6393 is pretty simple. As cache key is generated for the arguments passed to WP_Query, it means a arguments with empty string for post type and post type set to post, will generate a different cache key. In this PR, it cases where post type was set to empty string, post types passed an as array and other cases would result in a duplicate query have considered and unit tests written for them.

If possible @rodionov201 can you test this PR / once this PR is committed to ensure it fixes your issue.

#46 @spacedmonkey
2 weeks ago

  • Keywords has-unit-tests commit added; dev-feedback changes-requested removed
  • Owner changed from pbearne to spacedmonkey

As the PR has been approved by multiple core committers, I am marking as this a ready to commit and assigning back to myself to commit.

I plan to commit this early next week.

#47 @spacedmonkey
11 days ago

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

In 58122:

Query: Improve cache key generation.

Query caching in WP_Query was added in [53941]. When this functionality was added to the WP_Query class, a number of edge cases were missed that would result in redundant duplicate queries. It was possible to pass parameters to WP_Query that would be different but would result in the same database query being generated. As the cache key is generated from a mixture of query arguments and the SQL query, this resulted in different cache keys for the same database query, resulting in unnecessary duplicate queries. In this change, the logic in the generate_cache_key method has been improved to ensure reuse of existing caches. The following edge cases have been considered:

  • Passing post_type as an empty string.
  • Passing post_type as the string any.
  • Passing post_type as array vs. string.
  • Passing post_type as an array in a different order.
  • Not passing orderby.
  • Passing post_status as an array vs. string.
  • Passing post_status as an array in a different order.

This change also fixes an issue where the old SQL query would not match, as the queries had different whitespaces.

Props spacedmonkey, joemcgill, pbearne, peterwilsoncc, rajinsharwar, mukesh27, thekt12, huzaifaalmesbah, rodionov201.
Fixes #59442.

Note: See TracTickets for help on using tickets.