Make WordPress Core

Opened 21 months ago

Closed 4 months ago

Last modified 3 months ago

#61097 closed enhancement (fixed)

Filter `wp_count_posts()` query before execution to avoid slow query

Reported by: rcorrales's profile rcorrales Owned by: westonruter's profile westonruter
Milestone: 6.9 Priority: normal
Severity: normal Version: 2.5
Component: Posts, Post Types Keywords: has-patch has-unit-tests has-test-info commit has-dev-note add-to-field-guide
Focuses: performance Cc:

Description

Queries generated by the wp_count_posts() function for users without the read_private_posts capability incorporate the following conditions:

<?php
if ( ! current_user_can( $post_type_object->cap->read_private_posts ) ) {
        $query .= $wpdb->prepare(
                " AND (post_status != 'private' OR ( post_author = %d AND post_status = 'private' ))",
                get_current_user_id()
        );
}

This doesn't efficiently use indexes and makes the query extremely slow if there are millions of records in the wp_posts table.

One way to fix this could be to split the query:

SELECT post_status, COUNT(*) AS num_posts
FROM (
    SELECT post_status
    FROM wp_posts
    WHERE post_type = %s AND post_status != 'private'
    UNION ALL
    SELECT post_status
    FROM wp_posts
    WHERE post_type = %s AND post_status = 'private' AND post_author = %d
) AS filtered_posts
GROUP BY post_status;

In my tests with a table with +14M records, query time went from 8 minutes to 11 seconds, generating the same results. But I haven't seen any examples of UNION ALL operators in core, even though MySQL has supported them for a long time (since the early 2000s). I'm unsure if it's by design or if there simply hasn't been a need for that.

If modifying the query like that is not possible, could we add a pre_wp_count_posts filter before executing it so it can be overridden?

Change History (31)

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


20 months ago

#2 @mukesh27
20 months ago

  • Keywords needs-patch dev-feedback added
  • Version 6.5 deleted

#3 @matteoenna
20 months ago

  • Keywords needs-patch dev-feedback removed
  • Version set to 6.5

I'll start working on it

#4 @johnbillion
20 months ago

  • Component changed from Query to Posts, Post Types
  • Keywords needs-patch added

@rcorrales Thank you for the ticket!

This was briefly discussed during the performance team chat today and we agreed to add a filter for this query. Would you like to open a separate ticket for discussing the improvement to the query itself? That will need further discussion, unit test coverage, and performance reports. Thanks!

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


19 months ago
#5

  • Keywords has-patch added; needs-patch removed

## Ticket
https://core.trac.wordpress.org/ticket/61097

## Description

  • This PR proposes optimization to the wp_count_posts() function to improve query performance, especially for users without the read_private_posts capability.
  • The current implementation of the function's query includes inefficient conditions, leading to slow queries, particularly in databases with millions of records.
  • The proposed solution involves replacing the existing query logic with an optimized UNION ALL query, which separates the selection of non-private posts and private posts authored by the current user.
  • Additionally, the PR ensures backward compatibility by maintaining the original logic when the conditions for optimization are not met.

## Changes Made

  • Replaced the existing query logic in the wp_count_posts() function with an optimized UNION ALL query.
  • Incorporated conditional logic to use the optimized query only when necessary, falling back to the original logic otherwise.

## Context
The wp_count_posts() function is critical for counting posts of a specific type in WordPress. However, the current implementation suffers from performance issues due to inefficient query conditions. By optimizing the query with a UNION ALL approach, this PR aims to significantly improve performance, especially for large datasets.

@rcorrales commented on PR #6774:


19 months ago
#6

Referencing this PR on a new ticket focused on the performance of this query:
https://core.trac.wordpress.org/ticket/61502

#7 @rcorrales
19 months ago

@johnbillion I went ahead and created a separate ticket (#61502) for focusing on the performance improvements, while we add a new filter for this query using this ticket.

@snehapatil02 Thanks for submitting the PR! I referenced it in a comment in the new ticket.

It seems like it can't be added to the "Pull Requests" section of the new ticket unless it's mentioned in the PR description.

@snehapatil02 commented on PR #6774:


19 months ago
#8

@pbearne Fixed the phpcs errors.

@snehapatil02 commented on PR #6774:


19 months ago
#9

@pbearne Do I need to make any more changes??

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


4 months ago
#10

  • Keywords has-unit-tests added

Following the PR #6774 that was faulty but was not failing UT because they were incomplete

  • I've introduced the corresponding UT covering this section
  • Also added the filter, I see no point on having to separate tickets for such slight incremental improvement
  • Also I've slightly improved the post.php test file to better accomodate to the use of multiple user roles.

#11 @SirLouen
4 months ago

  • Keywords has-test-info needs-testing added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 6.5 to 2.5

This underperforming query was added in #6052/[7109]

Testing Information

Given that this is mostly a performance improvement, to test this is required to add a massive volume of posts. For example, add 100,000 private posts with an administration user and 100,000 normal public posts with the same administration user.

Then execute the wp_count_posts function with $priv readable but with a different user (with author capabilities). The result should be the 50,000 posts, but the important thing here is to measure the time to process with microtime with and without the patch.

#12 @SirLouen
4 months ago

#61502 was marked as a duplicate.

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-test by nikunj8866. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


4 months ago

#16 @sajjad67
4 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/9772

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.29
  • Server: nginx/1.29.1
  • Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 140.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.3
  • MU Plugins: None activated
  • Plugins:
    • WP Count Posts Tester 1.0
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

Before Patch:

Test (1) Results:

Starting tests...
Test 1: 0.868772 seconds
Test 2: 0.811469 seconds
Test 3: 0.841343 seconds
Test 4: 0.844959 seconds
Test 5: 0.936868 seconds
Test 6: 0.984893 seconds
Test 7: 0.992773 seconds
Test 8: 0.840826 seconds
Test 9: 0.844373 seconds
Test 10: 0.832345 seconds

Average Execution Time: 0.879862 seconds

Maximum Execution Time: 0.992773 seconds

Minimum Execution Time: 0.811469 seconds

Test (2) Results:

Starting tests...
Test 1: 0.891964 seconds
Test 2: 0.842310 seconds
Test 3: 0.871342 seconds
Test 4: 0.813705 seconds
Test 5: 0.826535 seconds
Test 6: 0.856882 seconds
Test 7: 0.833914 seconds
Test 8: 0.839651 seconds
Test 9: 0.819822 seconds
Test 10: 0.837396 seconds

Average Execution Time: 0.843352 seconds

Maximum Execution Time: 0.891964 seconds

Minimum Execution Time: 0.813705 seconds

After Patch:

Test (1) Results:

Starting tests...
Test 1: 0.179157 seconds
Test 2: 0.185003 seconds
Test 3: 0.199899 seconds
Test 4: 0.176707 seconds
Test 5: 0.160848 seconds
Test 6: 0.178117 seconds
Test 7: 0.167298 seconds
Test 8: 0.163988 seconds
Test 9: 0.164013 seconds
Test 10: 0.162798 seconds

Average Execution Time: 0.173783 seconds

Maximum Execution Time: 0.199899 seconds

Minimum Execution Time: 0.160848 seconds

Test (2) Results:

Starting tests...
Test 1: 0.163915 seconds
Test 2: 0.166620 seconds
Test 3: 0.153695 seconds
Test 4: 0.153376 seconds
Test 5: 0.149429 seconds
Test 6: 0.153423 seconds
Test 7: 0.154962 seconds
Test 8: 0.172091 seconds
Test 9: 0.214494 seconds
Test 10: 0.157147 seconds

Average Execution Time: 0.163915 seconds

Maximum Execution Time: 0.214494 seconds

Minimum Execution Time: 0.149429 seconds

Additional Notes

  • Huge difference in execution time
  • 2,000,000 Public Posts & 2,000,000 Private Posts, total (4,000,000 Posts) was used to test the execution time.

Supplemental Artifacts

https://img001.prntscr.com/file/img001/PDM6uqPMQoKdf88tzn8Aog.png

https://img001.prntscr.com/file/img001/5_AHEl-5S26AAZSFG4bNAw.png

https://img001.prntscr.com/file/img001/BuXAkWB5QNORRSsn8iJY5Q.png

https://img001.prntscr.com/file/img001/ng-h-Uc4Sa6F1eWCkvxrdA.png

This ticket was mentioned in Slack in #core-test by sajjad67. View the logs.


4 months ago

#18 @SirLouen
4 months ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 6.9

This has been fully cooked and is ready for delivery.
Thanks to @sajjad67 for thorough testing and @westonruter for review, and obviously @rcorrales for the report and the SQL proposal.

@westonruter commented on PR #9772:


4 months ago
#19

I asked Gemini to evaluate the old query compared with the new query for whether they are equivalent and why the UNION ALL version would be more efficient. What follows is the response:

Yes, the two queries are logically equivalent, and the second version using UNION ALL is generally more efficient.

Here’s a breakdown of why.


### Are they equivalent?

Yes. Both queries are designed to count posts for a user who does not have permission to view all private posts. They achieve the same result by selecting the same set of posts:

  1. All posts that are not private.
  2. All posts that are private but were written by the current user.
  • The first query accomplishes this using a single SELECT with a complex OR condition in the WHERE clause:
... WHERE post_type = %s AND (post_status != 'private' OR (post_author = %d AND post_status = 'private'))

This logic says, "for the given post type, find all rows where the status isn't private, OR where the status is private and the author is me."

  • The second query breaks this logic into two separate, simpler SELECT statements and combines their results:
  1. The first SELECT gets all posts where post_status != 'private'.
  2. The second SELECT gets all posts where post_status = 'private' AND post_author = %d.
  3. UNION ALL then merges these two result sets. The outer query then performs the COUNT.

Since the two sets of conditions (post_status != 'private' and post_status = 'private') are mutually exclusive, there's no overlap between the two SELECT statements in the UNION. Therefore, the final combined set of posts is identical to the set produced by the first query.


### Which is more efficient?

The second query using UNION ALL is likely more efficient.

The main reason comes down to how the MySQL query optimizer can use indexes.

  • Complex OR Conditions: The first query's WHERE ... AND (A OR B) structure can be difficult for the database optimizer. It might not be able to effectively use an index that covers both post_status and post_author, potentially leading it to scan more rows than necessary. The optimizer has to evaluate the complex OR logic for each row that matches the post_type.
  • Simple, Separate Queries: The UNION ALL approach creates two distinct, simple queries. Each of these queries has a straightforward WHERE clause that the optimizer can easily handle.
  1. ... WHERE post_type = %s AND post_status != 'private' can efficiently use an index on (post_type, post_status).
  2. ... WHERE post_type = %s AND post_status = 'private' AND post_author = %d can efficiently use an index on (post_type, post_status, post_author).

The database can execute these two highly optimized queries very quickly and then simply append the results. This is often much faster than trying to optimize a single query with a complex OR condition.

### Conclusion

While both queries produce the same result, the UNION ALL version is preferable. It is more "optimizer-friendly" by breaking a complex logical condition into two simpler parts, allowing MySQL to make better use of table indexes for faster execution. It's also arguably more readable, as it clearly separates the two distinct sets of data you're trying to retrieve.

#20 follow-up: @westonruter
4 months ago

The PR looks good.

I do have a question on whether the new filter is needed, however. Originally it was proposed as a way to fix the problem if the SQL query wasn't optimized. But the optimization with UNION ALL is applied. So is the filter still needed?

#21 in reply to: ↑ 20 @SirLouen
4 months ago

Replying to westonruter:

I asked Gemini to evaluate the old query compared with the new query for whether they are equivalent and why the UNION ALL version would be more efficient. What follows is the response:

Yes, the two queries are logically equivalent, and the second version using UNION ALL is generally more efficient.

https://i.imgur.com/XAe1uXc.png
Thorough performance testing
AI's Opinion

By the way, who in the world asks Gemini, having Claude available? Are you on Google's marketing team now? xD

The PR looks good.

I do have a question on whether the new filter is needed, however. Originally it was proposed as a way to fix the problem if the SQL query wasn't optimized. But the optimization with UNION ALL is applied. So is the filter still needed?

Now, on a serious note, yep, I also thought the same while building the test, you asked me. The test content started to look very awkward when trying to reproduce a possible real-life use-case, like, "build your own query within this function." I tried to put an example of a query that could make sense (counting children), which you could easily conclude that i could be executed anywhere without the filter… But @johnbillion asked @rcorrales back in the day to open a new ticket just for this, so I did not think deeply about the reason behind this and just added the amazing filter. The only real use could have been the possibility of extending the query, but with already the main keyword clauses in place (specially WHERE), this use is pretty much consumed.

Conclusion: I also believe that the filter should go, and I've removed it from the PR. If anyone wants a filter in the future, open a new ticket

So the patch is ready to be shipped again.

Last edited 4 months ago by SirLouen (previous) (diff)

#22 follow-up: @sajjad67
4 months ago

@SirLouen It is hilarious!! Lol!!

https://i.imgur.com/XAe1uXc.png

#23 @rcorrales
4 months ago

Thanks for implementing these changes and running all the tests!

In my tests with a table with +14M records, query time went from 8 minutes to 11 seconds, generating the same results.

I'd like to add that, as noted in the ticket description, I also verified the performance and results of the UNION ALL query using the query filter with millions of records at that time (and there should be at least a couple of large production sites running that workaround for a while as well).

I agree that a new filter is no longer required, since the root cause is being addressed, and further adjustments can still be made using other filters if needed.

#24 in reply to: ↑ 22 @SirLouen
4 months ago

Replying to sajjad67:

@SirLouen It is hilarious!! Lol!!

Disclaimer: I know that Weston looks like a fun guy, judging by his conferences. But be aware that not all could handle a little pun here and there. Caution and moderation at discretion. :)

#25 follow-up: @westonruter
4 months ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted

@SirLouen Naturally the performance tests speak for themselves, but I wanted to get an AI second opinion and sanity check on the query changes since I don't have much experience with UNION ALL as I recall. I've never used Claude, but I do have a Gemini subscription, so I generally use it exclusively for all my AI needs since I'm already paying for it.

(I guess using Gemini could have been construed as marketing in the past since I was employed by Google, but since I was laid off in April, I have no financial interest here. As I understand, Gemini and Claude are not so different in terms of capabilities. Nevertheless, I know you're just having a bit of fun.)

#26 @westonruter
4 months ago

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

In 60788:

Posts, Post Types: Improve wp_count_posts() query performance for users who cannot read_private_posts.

The query is refactored to use two subqueries which can leverage DB indexes.

Props rcorrales, snehapatil02, sirlouen, sajjad67, pbearne, johnbillion, westonruter.
Fixes #61097.

#27 in reply to: ↑ 25 @SirLouen
4 months ago

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

Replying to westonruter:

@SirLouen Naturally the performance tests speak for themselves, but I wanted to get an AI second opinion and sanity check on the query changes since I don't have much experience with UNION ALL as I recall.

I've added the corresponding `dev-note` to comment about this enhancement.

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


4 months ago
#28

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

  • Refactor post status query preparation
  • Minor changes in unit test

#29 @westonruter
4 months ago

In 60792:

Posts, Post Types: Refactor preparation of query for wp_count_posts().

Follow-up to [60788].

Props mukesh27, westonruter.
See #61097.

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


3 months ago

Note: See TracTickets for help on using tickets.