Duplicate query in WP_Query

[spacedmonkey]

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.

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

One nice side effect of this change, is this line will not have an empty string.

​https://github.com/WordPress/wordpress-develop/blob/4ae2d1a533c6f6dc3b7d82505763dc732924e0f0/src/wp-includes/class-wp-query.php#L3536

[hellofromTonya]

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?

[thekt12]

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

[thekt12]

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

[spacedmonkey]

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

[spacedmonkey]

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.

[joemcgill]

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

[spacedmonkey]

@joemcgill Answered feedback.

[peterwilsoncc]

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.

[spacedmonkey]

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

[peterwilsoncc]

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.

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

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.

[joemcgill]

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.

[spacedmonkey]

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

[huzaifaalmesbah]

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

[joemcgill]

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)

[joemcgill]

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

[pbearne]

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

[pbearne]

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

I am not sure this patch does anything

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

[spacedmonkey]

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

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!

Thanks for the feedback @peterwilsoncc . Actioned it here ​https://github.com/WordPress/wordpress-develop/pull/6393/commits/55026bcad3380ec1ee30e95379ba47be3c84994a

@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

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

[spacedmonkey]

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.

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

[spacedmonkey]

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.

Committed in https://core.trac.wordpress.org/changeset/58122

Committed in https://core.trac.wordpress.org/changeset/58122