WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 18 months ago

#14426 closed enhancement (fixed)

unneeded "SELECT FOUND_ROWS()" when no posts are found

Reported by: mark-k Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch
Focuses: Cc:

Description

the query is being executed even if no posts are found....

IMHO the lines in query.php

		if ( !empty($limits) ) {
			$found_posts_query = apply_filters( 'found_posts_query', 'SELECT FOUND_ROWS()' );
			$this->found_posts = $wpdb->get_var( $found_posts_query );

should be replaced with

		if ( !empty($limits) ) {
                   if (!empty($this->posts)) {
			$found_posts_query = apply_filters( 'found_posts_query', 'SELECT FOUND_ROWS()' );
			$this->found_posts = $wpdb->get_var( $found_posts_query );
                   } else 
                       $this->found_posts = 0;

Attachments (5)

14426.patch (981 bytes) - added by SergeyBiryukov 20 months ago.
14426.2.patch (1.8 KB) - added by SergeyBiryukov 20 months ago.
14426.3.patch (2.3 KB) - added by SergeyBiryukov 20 months ago.
14426.4.patch (2.8 KB) - added by SergeyBiryukov 19 months ago.
14426.diff (588 bytes) - added by ryan 18 months ago.

Download all attachments as: .zip

Change History (30)

comment:1 mark-k4 years ago

  • Summary changed from unneede "SELECT FOUND_ROWS()" when no posts are found to unneeded "SELECT FOUND_ROWS()" when no posts are found

comment:2 nacin4 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:3 follow-up: Denis-de-Bernardy4 years ago

Doesn't that filter allows to redo the query using a plugin?

comment:4 in reply to: ↑ 3 mark-k4 years ago

Replying to Denis-de-Bernardy:

Doesn't that filter allows to redo the query using a plugin?

If that is the purpose then why is it depended on $limit being non zero?
Even if it is, then the proposed code should probably be

		if ( !empty($limits) ) {
		   $found_posts_query = apply_filters( 'found_posts_query', 'SELECT FOUND_ROWS()' );
                   if (!empty($this->posts) || ($found_posts_query != 'SELECT FOUND_ROWS()') 
			$this->found_posts = $wpdb->get_var( $found_posts_query );
                   else 
                       $this->found_posts = 0;

comment:5 tmoorewp4 years ago

  • Cc tim@… added

Is this issue already fixed? I'm seeing the following in my wp-includes/query.php file:

		if ( !$q['no_found_rows'] && !empty($limits) ) {
			$found_posts_query = apply_filters_ref_array( 'found_posts_query', array( 'SELECT FOUND_ROWS()', &$this ) );
			$this->found_posts = $wpdb->get_var( $found_posts_query );
			$this->found_posts = apply_filters_ref_array( 'found_posts', array( $this->found_posts, &$this ) );
			$this->max_num_pages = ceil($this->found_posts / $q['posts_per_page']);
		}

The additional !$q['no_found_rows'] should prevent the extra SELECT FOUND_ROWS() from running, correct?

comment:6 mark-k4 years ago

@tmoorewp, IMO no. It doesn't answer the situation in which I would generally like to get the number of rows, but at a specific query (a search for example) there are simply no matching results.

comment:7 jane3 years ago

  • Keywords needs-patch added

There are a few days left before freeze. If someone wants to post a patch to get this in, now's the time; otherwise it will be punted to the 3.2 dev cycle.

comment:8 markjaquith3 years ago

  • Milestone changed from 3.1 to Future Release

Punt.

comment:10 follow-up: wonderboymusic20 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

I no longer see the SELECT FOUND_ROWS() happening after empty query results

add_filter( 'query', function ( $sql ) { error_log( $sql ); return $sql; } );
$q = new WP_Query( array( 'post__in' => array( 0 ) ) );

comment:11 in reply to: ↑ 10 SergeyBiryukov20 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Replying to wonderboymusic:

I no longer see the SELECT FOUND_ROWS() happening after empty query results

This is partly fixed in [19918], but only if $split_the_query is true:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/query.php#L2630

If $split_the_query is false, set_found_posts() still runs regardless of empty results.

comment:12 nacin20 months ago

  • Component changed from Optimization to Query
  • Milestone changed from Future Release to 3.5

set_found_posts() should check if ( $this->posts ), and otherwise set 0 for found_posts and max_num_pages. We shouldn't need to do $this->found_posts = $this->max_num_pages = 0; manually.

SergeyBiryukov20 months ago

SergeyBiryukov20 months ago

comment:13 SergeyBiryukov20 months ago

  • Keywords has-patch added; needs-patch removed

found_posts is 0 by default, so I guess set_found_posts() can just return early if ! $this->posts (14426.patch).

14426.2.patch is a combined patch for #17066 and #21147 as well.

SergeyBiryukov20 months ago

comment:14 SergeyBiryukov20 months ago

14426.3.patch also sets post_count when fields parameter is set (mentioned in #21733).

comment:15 SergeyBiryukov20 months ago

$found_posts description seems inaccurate:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/query.php#L981

Currently, it's the amount of posts if limit clause _was_ used (unless I'm missing something).

comment:16 wonderboymusic19 months ago

#17066 was marked as a duplicate.

comment:13 wonderboymusic19 months ago

#21147 was marked as a duplicate.

comment:14 nacin19 months ago

$found_posts description seems inaccurate

Yes, it is. It is meant to only be set when limits are *are* used. However, we should also set it when limits are *not* used, since it would be equal to post_count in this case.

We should also be able to avoid a FOUND ROWS query when the post_count is less than what the limit requested, but that's a bit more difficult as you also have to ensure there was no offset or paging in effect.

14426.3.patch looks good. Do we agree?

SergeyBiryukov19 months ago

comment:15 SergeyBiryukov19 months ago

14426.4.patch also updates $found_posts description and adds PHPDocs for set_found_posts().

comment:16 ryan19 months ago

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

In [21928]:

  • Avoid FOUND ROWS when no posts are found
  • Set post_count and found_posts for all 'fields' queries.
  • Set found_posts to post_count when limits are not used
  • Update phpdoc for $found_posts and set_found_posts()

Props SergeyBiryukov, wonderboymusic

fixes #14426

comment:17 nacin19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[21928] broke the advanced caching plugin. That's an older version than what they run on WP.com, but you can see that the found_posts_query is avoided if the query is cached, and then on the the_posts hook, the posts are populated. Right after the_posts, core sets post_count.

If we're going to avoid setting found_posts if no posts are found, then we may need to re-adjust found_posts when we adjust post_count after the_posts. If we can't figure out the right logic, we should just remove || ! $this->posts for now and come back to it in 3.6.

comment:18 follow-up: ryan18 months ago

Or have the plugin call set_found_posts() itself. There's only so much that can be done to retain back compat with a plugin that does such deep voodoo.

comment:19 in reply to: ↑ 18 nacin18 months ago

Replying to ryan:

Or have the plugin call set_found_posts() itself. There's only so much that can be done to retain back compat with a plugin that does such deep voodoo.

Or we can type-check it. So, return if $this->posts === array() (or is_array( $this->posts ) && ! $this->posts). That allows a null/false for $this->posts to be considered as "hey, someone is doing some crazy stuff here, let's not make too many assumptions."

ryan18 months ago

comment:20 ryan18 months ago

A caching plugin that sets posts to an empty array would sneak through, but I doubt there are any of those.

comment:21 ryan18 months ago

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

In [22258]:

If posts is an empty array, bail from set_found_posts(). If posts is null or otherwise empty proceed through set_found_posts(). This accommodates caching plugins such as Advanced Post Cache that force posts to be empty for later population but still require the found_posts_query filter to run.

fixes #14426

Note: See TracTickets for help on using tickets.