Make WordPress Core

Opened 12 years ago

Closed 10 years ago

#29552 closed enhancement (worksforme)

Slight tweak to no_found_rows logic in WP_Query

Reported by: dmchale Owned by: boonebgorges
Priority: normal Milestone:
Component: Query Version: 1.5
Severity: normal Keywords: reporter-feedback needs-testing has-patch
Cc: Focuses: performance

Description (last modified by johnbillion)

It seems that edge cases could introduce times when a query WOULD have limits but NOT want them (when the query was passed nopaging=true or post_per_page=-1). When this happened, SQL_CALC_FOUND_ROWS would be invoked in the query and hinder performance of the query.

Code change still allows for a developer to override no_found_rows with their own value, but now checks the nopaging and post_per_page variables to possibly set no_found_rows to true anyway. By using one of those 2 variables to ask for EVERYTHING, we know the developer does not want/need pagination. This forces the issue so that by the time we get to the line...

if ( !$q['no_found_rows'] && !empty($limits) )

...we don't find ourselves WITH limits, and a False value for no_found_rows, in spite of the fact that the developer had asked for "all".

Attachments (2)

29552.patch​ (782 bytes ) - added by dmchale 12 years ago.
29552.tests.patch​ (2.2 KB ) - added by boonebgorges 12 years ago.

Download all attachments as: .zip

Change History (10)

@dmchale
12 years ago

#1 @boonebgorges
12 years ago

  • Keywords reporter-feedback added

Thanks for the patch, dmchale.

It seems that edge cases could introduce times when a query WOULD have limits but NOT want them (when the query was passed nopaging=true or post_per_page=-1)

I can't reproduce this. Check out the last two tests in 29552.tests.patch​. They declare nopaging=true and posts_per_page=-1, and in each case, you can see that the request query does *not* contain SQL_CALC_FOUND_ROWS. When you have LIMIT clauses, WP_Query does not append the SQL_CALC_FOUND_ROWS keyword. See https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php?annotate=blame#L3376. See also https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php?annotate=blame#L3611 to see how found_posts is set in these cases.

Can you say more about your "edge cases"?

#2 @johnbillion
12 years ago

  • Description modified (diff)
  • Keywords needs-testing added
  • Version trunk → 1.5

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


10 years ago

#4 @boonebgorges
10 years ago

  • Milestone Awaiting Review → 4.6
  • Owner set to boonebgorges
  • Status new → assigned

Assigning to myself to review the tests. If they are passing, I'll commit them and close the ticket as invalid, pending further details. If they're not passing, then maybe we have a bug on our hands.

#5 @DrewAPicture
10 years ago

  • Keywords has-patch added

#6 @ajv9540
10 years ago

  • Resolution → worksforme
  • Status assigned → closed

#7 @ocean90
10 years ago

  • Resolution worksforme
  • Status closed → reopened

#8 @boonebgorges
10 years ago

  • Milestone 4.6
  • Resolution → worksforme
  • Status reopened → closed

[37600] missed this ticket (fudged the commit message).

Tests seem OK to me, so I'm going to close as wontfix. If anyone has steps to reproduce, please feel free to reopen with details.

Note: See TracTickets for help on using tickets.