Make WordPress Core

Opened 5 weeks ago

Closed 4 weeks ago

#59224 closed defect (bug) (fixed)

get_pages: duplicate `WP_Query::get_posts` call

Reported by: davidbinda's profile david.binda Owned by: joemcgill's profile joemcgill
Milestone: 6.3.2 Priority: normal
Severity: normal Version: 6.3
Component: Posts, Post Types Keywords: has-patch commit fixed-major
Focuses: performance Cc:

Description (last modified by SergeyBiryukov)

The internal usage of WP_Query in get_pages (introduced in r55569 ) performs the WP_Query::get_posts() call twice ( see source:tags/6.3/src/wp-includes/post.php#L6094 ).

When query args are passed to the WP_Query constructor, it returns the result of the WP_Query::get_posts method call (proxied through WP_Query::query). All the fetched posts are then already present in the WP_Query object and the correct way to access those is via the WP_Query->posts property.

In case the WP_Query::get_posts() is used, a duplicate SQL query is performed, eventually leading to performance downgrade (yet the caching inside the WP_Query mitigates this to some degree, unless cache_results query param is set to true).

Anyway, I would propose to work with the WP_Query inside the get_pages in the similar way as the code does in the get_posts, where the query args are passed to the object through the WP_Query::query method it's return value (the found posts) is being used (see source:tags/6.3/src/wp-includes/post.php#L2440 ).

Attachments (1)

59224.diff (536 bytes) - added by david.binda 5 weeks ago.

Download all attachments as: .zip

Change History (15)

@david.binda
5 weeks ago

#1 @SergeyBiryukov
5 weeks ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 6.4

#2 @azaozz
5 weeks ago

  • Milestone changed from 6.4 to 6.3.2

Uh, yea, I always suspected that [55569] will backfire eventually.

WP pages are not "just another post type", they have some specifics that all current code is expecting. Throwing away 14 years of incremental fixes and improvements for pages is not a good decision imho.

Changing the milestone to 6.3.2 as it would be best to fix this soon.

Last edited 5 weeks ago by azaozz (previous) (diff)

#3 @mukesh27
5 weeks ago

Thank you, @davidbinda, for providing the ticket and patch.

As you mentioned, the lack of extra query requests due to the wp_query cache explains why this went unnoticed. Well done on catching that detail.

The proposed changes are looks good to me.

#4 @spacedmonkey
5 weeks ago

  • Keywords commit added

Good catch @davidbinda.

Patch looks good to me. If other are happy, I will commit.

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


5 weeks ago

#6 @flixos90
5 weeks ago

Thanks @davidbinda, great catch! Fix looks good to me as well.

#7 @SergeyBiryukov
5 weeks ago

The patch looks good to me as well.

This ticket was mentioned in PR #5114 on WordPress/wordpress-develop by joemcgill.


5 weeks ago
#8

This is a direct copy of 59224.diff from the original ticket. Opening the PR to allow CI to run before committing.

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

#9 @joemcgill
5 weeks ago

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

#10 @joemcgill
5 weeks ago

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

In 56491:

Posts, Post Types: Avoid redundant SQL query in get_pages().

This avoids an additional query by passing the query args directly to the WP_Query::query() method, rather than to the constructor and calling get_posts(), following [55569].

Props david.binda, azaozz, spacedmonkey, mukesh27, flixos90, SergeyBiryukov, joemcgill.
Fixes #59224.

#11 @joemcgill
5 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to be backported to the 6.3 branch.

joemcgill commented on PR #5114:


5 weeks ago
#12

Committed in 56491.

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


5 weeks ago

#14 @flixos90
4 weeks ago

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

In 56516:

Posts, Post Types: Avoid redundant SQL query in get_pages().

This avoids an additional query by passing the query args directly to the WP_Query::query() method, rather than to the constructor and calling get_posts(), following [55569].

Props david.binda, azaozz, spacedmonkey, mukesh27, flixos90, SergeyBiryukov, joemcgill.
Merges [56491] to the 6.3 branch.
Fixes #59224.

Note: See TracTickets for help on using tickets.